Re: [PATCH] virtio-pci: add softlinks between virtio and pci

Re: [PATCH] virtio-pci: add softlinks between virtio and pci

[Anthony Liguori]

On 01/05/2011 01:17 PM, Michael S. Tsirkin wrote:
> We sometimes need to map between the virtio device and
> the given pci device. One such use is OS installer that
> gets the boot pci device from BIOS and needs to
> find the relevant block device. Since it can't,
> installation fails.
>    

I have no objection to this patch but I'm a tad confused by the description.

I assume you mean the installer is querying the boot device via int13 
get driver parameters such that it returns the pci address of the device?

Or is it querying geometry information and then trying to find the best 
match block device?

If it's the former, I don't really understand the need for a backlink 
since the PCI address gives you a link to the block device.  OTOH, if 
it's the later, it would make sense but then your description doesn't 
really make much sense.

At any rate, a better commit message would be helpful in explaining the 
need for this.

Regards,

Anthony Liguori

> Supply softlinks between these to make it possible.
>
> Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
> ---
>
> Gleb, could you please ack that this patch below
> will be enough to fix the installer issue that
> you see?
>
>   drivers/virtio/virtio_pci.c |   18 +++++++++++++++++-
>   1 files changed, 17 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> index ef8d9d5..06eb2f8 100644
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -25,6 +25,7 @@
>   #include<linux/virtio_pci.h>
>   #include<linux/highmem.h>
>   #include<linux/spinlock.h>
> +#include<linux/sysfs.h>
>
>   MODULE_AUTHOR("Anthony Liguori<aliguori@us.ibm.com>");
>   MODULE_DESCRIPTION("virtio-pci");
> @@ -667,8 +668,21 @@ static int __devinit virtio_pci_probe(struct pci_dev *pci_dev,
>   	if (err)
>   		goto out_set_drvdata;
>
> -	return 0;
> +	err = sysfs_create_link(&pci_dev->dev.kobj,&vp_dev->vdev.dev.kobj,
> +				"virtio_device");
> +	if (err)
> +		goto out_register_device;
> +
> +	err = sysfs_create_link(&vp_dev->vdev.dev.kobj,&pci_dev->dev.kobj,
> +				"bus_device");
> +	if (err)
> +		goto out_create_link;
>
> +	return 0;
> +out_create_link:
> +	sysfs_remove_link(&pci_dev->dev.kobj, "virtio_device");
> +out_register_device:
> +	unregister_virtio_device(&vp_dev->vdev);
>   out_set_drvdata:
>   	pci_set_drvdata(pci_dev, NULL);
>   	pci_iounmap(pci_dev, vp_dev->ioaddr);
> @@ -685,6 +699,8 @@ static void __devexit virtio_pci_remove(struct pci_dev *pci_dev)
>   {
>   	struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
>
> +	sysfs_remove_link(&vp_dev->vdev.dev.kobj, "bus_device");
> +	sysfs_remove_link(&pci_dev->dev.kobj, "virtio_device");
>   	unregister_virtio_device(&vp_dev->vdev);
>   }
>
>    

Re: [PATCH] virtio-pci: add softlinks between virtio and pci

[Michael S. Tsirkin]

On Wed, Jan 05, 2011 at 01:28:34PM -0600, Anthony Liguori wrote:
> On 01/05/2011 01:17 PM, Michael S. Tsirkin wrote:
> >We sometimes need to map between the virtio device and
> >the given pci device. One such use is OS installer that
> >gets the boot pci device from BIOS and needs to
> >find the relevant block device. Since it can't,
> >installation fails.
> 
> I have no objection to this patch but I'm a tad confused by the description.
> 
> I assume you mean the installer is querying the boot device via
> int13 get driver parameters such that it returns the pci address of
> the device?
> 
> Or is it querying geometry information and then trying to find the
> best match block device?

I think it's the former.

> If it's the former, I don't really understand the need for a
> backlink since the PCI address gives you a link to the block device.

It does? How does it?

> OTOH, if it's the later, it would make sense but then your
> description doesn't really make much sense.
> 
> At any rate, a better commit message would be helpful in explaining
> the need for this.
> 
> Regards,
> 
> Anthony Liguori
> 
> >Supply softlinks between these to make it possible.
> >
> >Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
> >---
> >
> >Gleb, could you please ack that this patch below
> >will be enough to fix the installer issue that
> >you see?
> >
> >  drivers/virtio/virtio_pci.c |   18 +++++++++++++++++-
> >  1 files changed, 17 insertions(+), 1 deletions(-)
> >
> >diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> >index ef8d9d5..06eb2f8 100644
> >--- a/drivers/virtio/virtio_pci.c
> >+++ b/drivers/virtio/virtio_pci.c
> >@@ -25,6 +25,7 @@
> >  #include<linux/virtio_pci.h>
> >  #include<linux/highmem.h>
> >  #include<linux/spinlock.h>
> >+#include<linux/sysfs.h>
> >
> >  MODULE_AUTHOR("Anthony Liguori<aliguori@us.ibm.com>");
> >  MODULE_DESCRIPTION("virtio-pci");
> >@@ -667,8 +668,21 @@ static int __devinit virtio_pci_probe(struct pci_dev *pci_dev,
> >  	if (err)
> >  		goto out_set_drvdata;
> >
> >-	return 0;
> >+	err = sysfs_create_link(&pci_dev->dev.kobj,&vp_dev->vdev.dev.kobj,
> >+				"virtio_device");
> >+	if (err)
> >+		goto out_register_device;
> >+
> >+	err = sysfs_create_link(&vp_dev->vdev.dev.kobj,&pci_dev->dev.kobj,
> >+				"bus_device");
> >+	if (err)
> >+		goto out_create_link;
> >
> >+	return 0;
> >+out_create_link:
> >+	sysfs_remove_link(&pci_dev->dev.kobj, "virtio_device");
> >+out_register_device:
> >+	unregister_virtio_device(&vp_dev->vdev);
> >  out_set_drvdata:
> >  	pci_set_drvdata(pci_dev, NULL);
> >  	pci_iounmap(pci_dev, vp_dev->ioaddr);
> >@@ -685,6 +699,8 @@ static void __devexit virtio_pci_remove(struct pci_dev *pci_dev)
> >  {
> >  	struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
> >
> >+	sysfs_remove_link(&vp_dev->vdev.dev.kobj, "bus_device");
> >+	sysfs_remove_link(&pci_dev->dev.kobj, "virtio_device");
> >  	unregister_virtio_device(&vp_dev->vdev);
> >  }
> >

Re: [PATCH] virtio-pci: add softlinks between virtio and pci

[Michael S. Tsirkin]

On Wed, Jan 05, 2011 at 01:28:34PM -0600, Anthony Liguori wrote:
> On 01/05/2011 01:17 PM, Michael S. Tsirkin wrote:
> >We sometimes need to map between the virtio device and
> >the given pci device. One such use is OS installer that
> >gets the boot pci device from BIOS and needs to
> >find the relevant block device. Since it can't,
> >installation fails.
> 
> I have no objection to this patch but I'm a tad confused by the description.
> 
> I assume you mean the installer is querying the boot device via
> int13 get driver parameters such that it returns the pci address of
> the device?
> 
> Or is it querying geometry information and then trying to find the
> best match block device?
> 
> If it's the former, I don't really understand the need for a
> backlink since the PCI address gives you a link to the block device.
> OTOH, if it's the later, it would make sense but then your
> description doesn't really make much sense.
> 
> At any rate, a better commit message would be helpful in explaining
> the need for this.
> 
> Regards,
> 
> Anthony Liguori

OK just to clarify: we get pci address from BIOS
and need the virtio device to get at the linux device
(e.g. block) in the end.  Thus the link from pci to virtio.
I also added a backlink since I thought it's handy.

Does this answer the questions?

Rusty rewrites my commit logs anyway, he has better style :)

> >Supply softlinks between these to make it possible.
> >
> >Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
> >---
> >
> >Gleb, could you please ack that this patch below
> >will be enough to fix the installer issue that
> >you see?
> >
> >  drivers/virtio/virtio_pci.c |   18 +++++++++++++++++-
> >  1 files changed, 17 insertions(+), 1 deletions(-)
> >
> >diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> >index ef8d9d5..06eb2f8 100644
> >--- a/drivers/virtio/virtio_pci.c
> >+++ b/drivers/virtio/virtio_pci.c
> >@@ -25,6 +25,7 @@
> >  #include<linux/virtio_pci.h>
> >  #include<linux/highmem.h>
> >  #include<linux/spinlock.h>
> >+#include<linux/sysfs.h>
> >
> >  MODULE_AUTHOR("Anthony Liguori<aliguori@us.ibm.com>");
> >  MODULE_DESCRIPTION("virtio-pci");
> >@@ -667,8 +668,21 @@ static int __devinit virtio_pci_probe(struct pci_dev *pci_dev,
> >  	if (err)
> >  		goto out_set_drvdata;
> >
> >-	return 0;
> >+	err = sysfs_create_link(&pci_dev->dev.kobj,&vp_dev->vdev.dev.kobj,
> >+				"virtio_device");
> >+	if (err)
> >+		goto out_register_device;
> >+
> >+	err = sysfs_create_link(&vp_dev->vdev.dev.kobj,&pci_dev->dev.kobj,
> >+				"bus_device");
> >+	if (err)
> >+		goto out_create_link;
> >
> >+	return 0;
> >+out_create_link:
> >+	sysfs_remove_link(&pci_dev->dev.kobj, "virtio_device");
> >+out_register_device:
> >+	unregister_virtio_device(&vp_dev->vdev);
> >  out_set_drvdata:
> >  	pci_set_drvdata(pci_dev, NULL);
> >  	pci_iounmap(pci_dev, vp_dev->ioaddr);
> >@@ -685,6 +699,8 @@ static void __devexit virtio_pci_remove(struct pci_dev *pci_dev)
> >  {
> >  	struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
> >
> >+	sysfs_remove_link(&vp_dev->vdev.dev.kobj, "bus_device");
> >+	sysfs_remove_link(&pci_dev->dev.kobj, "virtio_device");
> >  	unregister_virtio_device(&vp_dev->vdev);
> >  }
> >

Re: [PATCH] virtio-pci: add softlinks between virtio and pci

[Anthony Liguori]

On 01/05/2011 01:38 PM, Michael S. Tsirkin wrote:
> On Wed, Jan 05, 2011 at 01:28:34PM -0600, Anthony Liguori wrote:
>    
>> On 01/05/2011 01:17 PM, Michael S. Tsirkin wrote:
>>      
>>> We sometimes need to map between the virtio device and
>>> the given pci device. One such use is OS installer that
>>> gets the boot pci device from BIOS and needs to
>>> find the relevant block device. Since it can't,
>>> installation fails.
>>>        
>> I have no objection to this patch but I'm a tad confused by the description.
>>
>> I assume you mean the installer is querying the boot device via
>> int13 get driver parameters such that it returns the pci address of
>> the device?
>>
>> Or is it querying geometry information and then trying to find the
>> best match block device?
>>      
> I think it's the former.
>
>    
>> If it's the former, I don't really understand the need for a
>> backlink since the PCI address gives you a link to the block device.
>>      
> It does? How does it?
>    

Okay, after some more discussion, I think I better understand what's 
going on here.

The installer needs to figure out the boot device.  It does this by 
querying the BIOS and the BIOS can only give it back a PCI address.  
Right now, if you follow the PCI sysfs path, you cannot reach the actual 
virtio device because the PCI device only refers to the bus.  This is 
because virtio has a separate struct device than the PCI struct device.  
These explicit links establish the relationship.

A lot of this would be cleaner if virtio didn't force an independent 
struct device and could simply make use of an already existing struct 
device.

Regards,

Anthony Liguori

>    
>> OTOH, if it's the later, it would make sense but then your
>> description doesn't really make much sense.
>>
>> At any rate, a better commit message would be helpful in explaining
>> the need for this.
>>
>> Regards,
>>
>> Anthony Liguori
>>
>>      
>>> Supply softlinks between these to make it possible.
>>>
>>> Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
>>> ---
>>>
>>> Gleb, could you please ack that this patch below
>>> will be enough to fix the installer issue that
>>> you see?
>>>
>>>   drivers/virtio/virtio_pci.c |   18 +++++++++++++++++-
>>>   1 files changed, 17 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
>>> index ef8d9d5..06eb2f8 100644
>>> --- a/drivers/virtio/virtio_pci.c
>>> +++ b/drivers/virtio/virtio_pci.c
>>> @@ -25,6 +25,7 @@
>>>   #include<linux/virtio_pci.h>
>>>   #include<linux/highmem.h>
>>>   #include<linux/spinlock.h>
>>> +#include<linux/sysfs.h>
>>>
>>>   MODULE_AUTHOR("Anthony Liguori<aliguori@us.ibm.com>");
>>>   MODULE_DESCRIPTION("virtio-pci");
>>> @@ -667,8 +668,21 @@ static int __devinit virtio_pci_probe(struct pci_dev *pci_dev,
>>>   	if (err)
>>>   		goto out_set_drvdata;
>>>
>>> -	return 0;
>>> +	err = sysfs_create_link(&pci_dev->dev.kobj,&vp_dev->vdev.dev.kobj,
>>> +				"virtio_device");
>>> +	if (err)
>>> +		goto out_register_device;
>>> +
>>> +	err = sysfs_create_link(&vp_dev->vdev.dev.kobj,&pci_dev->dev.kobj,
>>> +				"bus_device");
>>> +	if (err)
>>> +		goto out_create_link;
>>>
>>> +	return 0;
>>> +out_create_link:
>>> +	sysfs_remove_link(&pci_dev->dev.kobj, "virtio_device");
>>> +out_register_device:
>>> +	unregister_virtio_device(&vp_dev->vdev);
>>>   out_set_drvdata:
>>>   	pci_set_drvdata(pci_dev, NULL);
>>>   	pci_iounmap(pci_dev, vp_dev->ioaddr);
>>> @@ -685,6 +699,8 @@ static void __devexit virtio_pci_remove(struct pci_dev *pci_dev)
>>>   {
>>>   	struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
>>>
>>> +	sysfs_remove_link(&vp_dev->vdev.dev.kobj, "bus_device");
>>> +	sysfs_remove_link(&pci_dev->dev.kobj, "virtio_device");
>>>   	unregister_virtio_device(&vp_dev->vdev);
>>>   }
>>>
>>>        

Re: [PATCH] virtio-pci: add softlinks between virtio and pci

[Anthony Liguori]

On 01/05/2011 02:05 PM, Michael S. Tsirkin wrote:
> On Wed, Jan 05, 2011 at 01:28:34PM -0600, Anthony Liguori wrote:
>    
>> On 01/05/2011 01:17 PM, Michael S. Tsirkin wrote:
>>      
>>> We sometimes need to map between the virtio device and
>>> the given pci device. One such use is OS installer that
>>> gets the boot pci device from BIOS and needs to
>>> find the relevant block device. Since it can't,
>>> installation fails.
>>>        
>> I have no objection to this patch but I'm a tad confused by the description.
>>
>> I assume you mean the installer is querying the boot device via
>> int13 get driver parameters such that it returns the pci address of
>> the device?
>>
>> Or is it querying geometry information and then trying to find the
>> best match block device?
>>
>> If it's the former, I don't really understand the need for a
>> backlink since the PCI address gives you a link to the block device.
>> OTOH, if it's the later, it would make sense but then your
>> description doesn't really make much sense.
>>
>> At any rate, a better commit message would be helpful in explaining
>> the need for this.
>>
>> Regards,
>>
>> Anthony Liguori
>>      
> OK just to clarify: we get pci address from BIOS
> and need the virtio device to get at the linux device
> (e.g. block) in the end.  Thus the link from pci to virtio.
> I also added a backlink since I thought it's handy.
>
> Does this answer the questions?
>
> Rusty rewrites my commit logs anyway, he has better style :)
>    

It helps.  The real reason this is needed is because in a normal device, 
there is only one struct device whereas with virtio-pci, the virtio-pci 
device has a struct device and then the actual virtio device has another 
one.  There's probably a better way to handle this in sysfs making 
virtio-pci a proper bus with only a single device as a child or 
something like that.

But the links are probably an easier solution.

Regards,

Anthony Liguori

>    
>>> Supply softlinks between these to make it possible.
>>>
>>> Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
>>> ---
>>>
>>> Gleb, could you please ack that this patch below
>>> will be enough to fix the installer issue that
>>> you see?
>>>
>>>   drivers/virtio/virtio_pci.c |   18 +++++++++++++++++-
>>>   1 files changed, 17 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
>>> index ef8d9d5..06eb2f8 100644
>>> --- a/drivers/virtio/virtio_pci.c
>>> +++ b/drivers/virtio/virtio_pci.c
>>> @@ -25,6 +25,7 @@
>>>   #include<linux/virtio_pci.h>
>>>   #include<linux/highmem.h>
>>>   #include<linux/spinlock.h>
>>> +#include<linux/sysfs.h>
>>>
>>>   MODULE_AUTHOR("Anthony Liguori<aliguori@us.ibm.com>");
>>>   MODULE_DESCRIPTION("virtio-pci");
>>> @@ -667,8 +668,21 @@ static int __devinit virtio_pci_probe(struct pci_dev *pci_dev,
>>>   	if (err)
>>>   		goto out_set_drvdata;
>>>
>>> -	return 0;
>>> +	err = sysfs_create_link(&pci_dev->dev.kobj,&vp_dev->vdev.dev.kobj,
>>> +				"virtio_device");
>>> +	if (err)
>>> +		goto out_register_device;
>>> +
>>> +	err = sysfs_create_link(&vp_dev->vdev.dev.kobj,&pci_dev->dev.kobj,
>>> +				"bus_device");
>>> +	if (err)
>>> +		goto out_create_link;
>>>
>>> +	return 0;
>>> +out_create_link:
>>> +	sysfs_remove_link(&pci_dev->dev.kobj, "virtio_device");
>>> +out_register_device:
>>> +	unregister_virtio_device(&vp_dev->vdev);
>>>   out_set_drvdata:
>>>   	pci_set_drvdata(pci_dev, NULL);
>>>   	pci_iounmap(pci_dev, vp_dev->ioaddr);
>>> @@ -685,6 +699,8 @@ static void __devexit virtio_pci_remove(struct pci_dev *pci_dev)
>>>   {
>>>   	struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
>>>
>>> +	sysfs_remove_link(&vp_dev->vdev.dev.kobj, "bus_device");
>>> +	sysfs_remove_link(&pci_dev->dev.kobj, "virtio_device");
>>>   	unregister_virtio_device(&vp_dev->vdev);
>>>   }
>>>
>>>        

Re: [PATCH] virtio-pci: add softlinks between virtio and pci

[Michael S. Tsirkin]

On Wed, Jan 05, 2011 at 02:08:33PM -0600, Anthony Liguori wrote:
> On 01/05/2011 02:05 PM, Michael S. Tsirkin wrote:
> >On Wed, Jan 05, 2011 at 01:28:34PM -0600, Anthony Liguori wrote:
> >>On 01/05/2011 01:17 PM, Michael S. Tsirkin wrote:
> >>>We sometimes need to map between the virtio device and
> >>>the given pci device. One such use is OS installer that
> >>>gets the boot pci device from BIOS and needs to
> >>>find the relevant block device. Since it can't,
> >>>installation fails.
> >>I have no objection to this patch but I'm a tad confused by the description.
> >>
> >>I assume you mean the installer is querying the boot device via
> >>int13 get driver parameters such that it returns the pci address of
> >>the device?
> >>
> >>Or is it querying geometry information and then trying to find the
> >>best match block device?
> >>
> >>If it's the former, I don't really understand the need for a
> >>backlink since the PCI address gives you a link to the block device.
> >>OTOH, if it's the later, it would make sense but then your
> >>description doesn't really make much sense.
> >>
> >>At any rate, a better commit message would be helpful in explaining
> >>the need for this.
> >>
> >>Regards,
> >>
> >>Anthony Liguori
> >OK just to clarify: we get pci address from BIOS
> >and need the virtio device to get at the linux device
> >(e.g. block) in the end.  Thus the link from pci to virtio.
> >I also added a backlink since I thought it's handy.
> >
> >Does this answer the questions?
> >
> >Rusty rewrites my commit logs anyway, he has better style :)
> 
> It helps.  The real reason this is needed is because in a normal
> device, there is only one struct device whereas with virtio-pci, the
> virtio-pci device has a struct device and then the actual virtio
> device has another one.

I like how it works with e.g. net devices:

$ ls -l /sys/class/net/eth1
lrwxrwxrwx 1 root root 0 Jan  5 23:10 /sys/class/net/eth1 -> ../../devices/pci0000:00/0000:00:19.0/net/eth1

We maybe could have had

lrwxrwxrwx 1 root root 0 Jan  5 23:10 /sys/bus/virtio/virtio0 -> ../../devices/pci0000:00/0000:00:19.0/virtio/virtio0

This is pretty and would preserve the compatibility, but I am not sure how to implement this:
bus seems to want to have real kobjs behind it, not softlinks.
Ideas?

> There's probably a better way to handle
> this in sysfs making virtio-pci a proper bus with only a single
> device as a child or something like that.

I admit I'm just confused by all these buses.

> 
> But the links are probably an easier solution.
> 
> Regards,
> 
> Anthony Liguori
> 
> >>>Supply softlinks between these to make it possible.
> >>>
> >>>Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
> >>>---
> >>>
> >>>Gleb, could you please ack that this patch below
> >>>will be enough to fix the installer issue that
> >>>you see?
> >>>
> >>>  drivers/virtio/virtio_pci.c |   18 +++++++++++++++++-
> >>>  1 files changed, 17 insertions(+), 1 deletions(-)
> >>>
> >>>diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> >>>index ef8d9d5..06eb2f8 100644
> >>>--- a/drivers/virtio/virtio_pci.c
> >>>+++ b/drivers/virtio/virtio_pci.c
> >>>@@ -25,6 +25,7 @@
> >>>  #include<linux/virtio_pci.h>
> >>>  #include<linux/highmem.h>
> >>>  #include<linux/spinlock.h>
> >>>+#include<linux/sysfs.h>
> >>>
> >>>  MODULE_AUTHOR("Anthony Liguori<aliguori@us.ibm.com>");
> >>>  MODULE_DESCRIPTION("virtio-pci");
> >>>@@ -667,8 +668,21 @@ static int __devinit virtio_pci_probe(struct pci_dev *pci_dev,
> >>>  	if (err)
> >>>  		goto out_set_drvdata;
> >>>
> >>>-	return 0;
> >>>+	err = sysfs_create_link(&pci_dev->dev.kobj,&vp_dev->vdev.dev.kobj,
> >>>+				"virtio_device");
> >>>+	if (err)
> >>>+		goto out_register_device;
> >>>+
> >>>+	err = sysfs_create_link(&vp_dev->vdev.dev.kobj,&pci_dev->dev.kobj,
> >>>+				"bus_device");
> >>>+	if (err)
> >>>+		goto out_create_link;
> >>>
> >>>+	return 0;
> >>>+out_create_link:
> >>>+	sysfs_remove_link(&pci_dev->dev.kobj, "virtio_device");
> >>>+out_register_device:
> >>>+	unregister_virtio_device(&vp_dev->vdev);
> >>>  out_set_drvdata:
> >>>  	pci_set_drvdata(pci_dev, NULL);
> >>>  	pci_iounmap(pci_dev, vp_dev->ioaddr);
> >>>@@ -685,6 +699,8 @@ static void __devexit virtio_pci_remove(struct pci_dev *pci_dev)
> >>>  {
> >>>  	struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
> >>>
> >>>+	sysfs_remove_link(&vp_dev->vdev.dev.kobj, "bus_device");
> >>>+	sysfs_remove_link(&pci_dev->dev.kobj, "virtio_device");
> >>>  	unregister_virtio_device(&vp_dev->vdev);
> >>>  }
> >>>

Re: [PATCH] virtio-pci: add softlinks between virtio and pci

[Gleb Natapov]

On Wed, Jan 05, 2011 at 09:17:11PM +0200, Michael S. Tsirkin wrote:
> We sometimes need to map between the virtio device and
> the given pci device. One such use is OS installer that
> gets the boot pci device from BIOS and needs to
> find the relevant block device. Since it can't,
> installation fails.
> 
> Supply softlinks between these to make it possible.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> 
> Gleb, could you please ack that this patch below
> will be enough to fix the installer issue that
> you see?
> 
ACK. With this patch given PCI address from EDD I can find device node
by looking at /sys/devices/pci0000:00/0000:edd_bdf/virtio_device/block/

>  drivers/virtio/virtio_pci.c |   18 +++++++++++++++++-
>  1 files changed, 17 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> index ef8d9d5..06eb2f8 100644
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -25,6 +25,7 @@
>  #include <linux/virtio_pci.h>
>  #include <linux/highmem.h>
>  #include <linux/spinlock.h>
> +#include <linux/sysfs.h>
>  
>  MODULE_AUTHOR("Anthony Liguori <aliguori@us.ibm.com>");
>  MODULE_DESCRIPTION("virtio-pci");
> @@ -667,8 +668,21 @@ static int __devinit virtio_pci_probe(struct pci_dev *pci_dev,
>  	if (err)
>  		goto out_set_drvdata;
>  
> -	return 0;
> +	err = sysfs_create_link(&pci_dev->dev.kobj, &vp_dev->vdev.dev.kobj,
> +				"virtio_device");
> +	if (err)
> +		goto out_register_device;
> +
> +	err = sysfs_create_link(&vp_dev->vdev.dev.kobj, &pci_dev->dev.kobj,
> +				"bus_device");
> +	if (err)
> +		goto out_create_link;
>  
> +	return 0;
> +out_create_link:
> +	sysfs_remove_link(&pci_dev->dev.kobj, "virtio_device");
> +out_register_device:
> +	unregister_virtio_device(&vp_dev->vdev);
>  out_set_drvdata:
>  	pci_set_drvdata(pci_dev, NULL);
>  	pci_iounmap(pci_dev, vp_dev->ioaddr);
> @@ -685,6 +699,8 @@ static void __devexit virtio_pci_remove(struct pci_dev *pci_dev)
>  {
>  	struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
>  
> +	sysfs_remove_link(&vp_dev->vdev.dev.kobj, "bus_device");
> +	sysfs_remove_link(&pci_dev->dev.kobj, "virtio_device");
>  	unregister_virtio_device(&vp_dev->vdev);
>  }
>  
> -- 
> 1.7.3.2.91.g446ac

--
			Gleb.

Re: virtio-pci: add softlinks between virtio and pci

[Milton Miller]

On: Wed, 05 Jan 2011 at about 19:17:11 -0000, Michael S. Tsirkin wrote:
> We sometimes need to map between the virtio device and
> the given pci device. One such use is OS installer that
> gets the boot pci device from BIOS and needs to
> find the relevant block device. Since it can't,
> installation fails.
> 

> Supply softlinks between these to make it possible.
> 
> Gleb, could you please ack that this patch below
> will be enough to fix the installer issue that
> you see?
> 

why not remove this device anchor and put the devices
under the pci device?   Proposed patch follows as a
reply.

milton

[PATCH] virtio: remove virtio-pci root device

[Milton Miller]

We sometimes need to map between the virtio device and
the given pci device. One such use is OS installer that
gets the boot pci device from BIOS and needs to
find the relevant block device. Since it can't,
installation fails.

Instead of creating a top-level devices/virtio-pci
directory, create each device under the corresponding
pci device node.  Symlinks to all virtio-pci
devices can be found under the pci driver link in
bus/pci/drivers/virtio-pci/devices, and all virtio
devices under drivers/bus/virtio/devices.

Signed-off-by: Milton Miller <miltonm@bga.com>
---

This is an alternative to the patch by Michael S. Tsirkin
titled "virtio-pci: add softlinks between virtio and pci"
https://patchwork.kernel.org/patch/454581/

It creates simpler code, uses less memory, and should
be even easier use by the installer as it won't have to
know a virtio symlink to follow (just follow none).

Compile tested only as I don't have kvm setup.


diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index ef8d9d5..4fb5b2b 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -96,11 +96,6 @@ static struct pci_device_id virtio_pci_id_table[] = {
 
 MODULE_DEVICE_TABLE(pci, virtio_pci_id_table);
 
-/* A PCI device has it's own struct device and so does a virtio device so
- * we create a place for the virtio devices to show up in sysfs.  I think it
- * would make more sense for virtio to not insist on having it's own device. */
-static struct device *virtio_pci_root;
-
 /* Convert a generic virtio device to our structure */
 static struct virtio_pci_device *to_vp_device(struct virtio_device *vdev)
 {
@@ -629,7 +624,7 @@ static int __devinit virtio_pci_probe(struct pci_dev *pci_dev,
 	if (vp_dev == NULL)
 		return -ENOMEM;
 
-	vp_dev->vdev.dev.parent = virtio_pci_root;
+	vp_dev->vdev.dev.parent = &pci_dev->dev;
 	vp_dev->vdev.dev.release = virtio_pci_release_dev;
 	vp_dev->vdev.config = &virtio_pci_config_ops;
 	vp_dev->pci_dev = pci_dev;
@@ -717,17 +712,7 @@ static struct pci_driver virtio_pci_driver = {
 
 static int __init virtio_pci_init(void)
 {
-	int err;
-
-	virtio_pci_root = root_device_register("virtio-pci");
-	if (IS_ERR(virtio_pci_root))
-		return PTR_ERR(virtio_pci_root);
-
-	err = pci_register_driver(&virtio_pci_driver);
-	if (err)
-		root_device_unregister(virtio_pci_root);
-
-	return err;
+	return pci_register_driver(&virtio_pci_driver);
 }
 
 module_init(virtio_pci_init);
@@ -735,7 +720,6 @@ module_init(virtio_pci_init);
 static void __exit virtio_pci_exit(void)
 {
 	pci_unregister_driver(&virtio_pci_driver);
-	root_device_unregister(virtio_pci_root);
 }
 
 module_exit(virtio_pci_exit);

Re: [PATCH] virtio: remove virtio-pci root device

[Michael S. Tsirkin]

On Fri, Jan 07, 2011 at 02:55:06AM -0600, Milton Miller wrote:
> We sometimes need to map between the virtio device and
> the given pci device. One such use is OS installer that
> gets the boot pci device from BIOS and needs to
> find the relevant block device. Since it can't,
> installation fails.
> 
> Instead of creating a top-level devices/virtio-pci
> directory, create each device under the corresponding
> pci device node.  Symlinks to all virtio-pci
> devices can be found under the pci driver link in
> bus/pci/drivers/virtio-pci/devices, and all virtio
> devices under drivers/bus/virtio/devices.
> 
> Signed-off-by: Milton Miller <miltonm@bga.com>

Thanks! I'll try this next week. 
Some comments from looking at code:

- This will break apps that look for devices in the old virtio-pci directory.
  There are not likely to be many of these,
  but better be careful. Let's create a compatibility
  option to put symlinks there?
  Could be under a compile time option with a deprecation plan.
  
- This will create ./virtio0 under the pci device, right?
  But if the device is then renamed, the installer
  won't be able to find it, right?
  And need to do pattern matching to find the name is a bit ugly.

Could we create ./virtio/virtio0 under the pci device?
Then one can open virtio directory and then the only file there.

> ---
> 
> This is an alternative to the patch by Michael S. Tsirkin
> titled "virtio-pci: add softlinks between virtio and pci"
> https://patchwork.kernel.org/patch/454581/
> 
> It creates simpler code, uses less memory, and should
> be even easier use by the installer as it won't have to
> know a virtio symlink to follow (just follow none).
> 
> Compile tested only as I don't have kvm setup.
> 
> 
> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> index ef8d9d5..4fb5b2b 100644
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -96,11 +96,6 @@ static struct pci_device_id virtio_pci_id_table[] = {
>  
>  MODULE_DEVICE_TABLE(pci, virtio_pci_id_table);
>  
> -/* A PCI device has it's own struct device and so does a virtio device so
> - * we create a place for the virtio devices to show up in sysfs.  I think it
> - * would make more sense for virtio to not insist on having it's own device. */
> -static struct device *virtio_pci_root;
> -
>  /* Convert a generic virtio device to our structure */
>  static struct virtio_pci_device *to_vp_device(struct virtio_device *vdev)
>  {
> @@ -629,7 +624,7 @@ static int __devinit virtio_pci_probe(struct pci_dev *pci_dev,
>  	if (vp_dev == NULL)
>  		return -ENOMEM;
>  
> -	vp_dev->vdev.dev.parent = virtio_pci_root;
> +	vp_dev->vdev.dev.parent = &pci_dev->dev;
>  	vp_dev->vdev.dev.release = virtio_pci_release_dev;
>  	vp_dev->vdev.config = &virtio_pci_config_ops;
>  	vp_dev->pci_dev = pci_dev;
> @@ -717,17 +712,7 @@ static struct pci_driver virtio_pci_driver = {
>  
>  static int __init virtio_pci_init(void)
>  {
> -	int err;
> -
> -	virtio_pci_root = root_device_register("virtio-pci");
> -	if (IS_ERR(virtio_pci_root))
> -		return PTR_ERR(virtio_pci_root);
> -
> -	err = pci_register_driver(&virtio_pci_driver);
> -	if (err)
> -		root_device_unregister(virtio_pci_root);
> -
> -	return err;
> +	return pci_register_driver(&virtio_pci_driver);
>  }
>  
>  module_init(virtio_pci_init);
> @@ -735,7 +720,6 @@ module_init(virtio_pci_init);
>  static void __exit virtio_pci_exit(void)
>  {
>  	pci_unregister_driver(&virtio_pci_driver);
> -	root_device_unregister(virtio_pci_root);
>  }
>  
>  module_exit(virtio_pci_exit);

Re: [PATCH] virtio: remove virtio-pci root device

[Michael S. Tsirkin]

On Fri, Jan 07, 2011 at 02:55:06AM -0600, Milton Miller wrote:
> We sometimes need to map between the virtio device and
> the given pci device. One such use is OS installer that
> gets the boot pci device from BIOS and needs to
> find the relevant block device. Since it can't,
> installation fails.
> 
> Instead of creating a top-level devices/virtio-pci
> directory, create each device under the corresponding
> pci device node.  Symlinks to all virtio-pci
> devices can be found under the pci driver link in
> bus/pci/drivers/virtio-pci/devices, and all virtio
> devices under drivers/bus/virtio/devices.
> 
> Signed-off-by: Milton Miller <miltonm@bga.com>

OK, this works fine for me.  I played with options to add compat
softlinks under devices/virtio-pci but we still don't get exactly the
same layout and since I don't think anyone actually uses them, it's
probably ok to just to the simple thing.

Tested/Acked-by: Michael S. Tsirkin <mst@redhat.com>

Rusty, since this help fix at least one user, any chance this can be put
in 2.6.38? OK to backport to -stable?

Gleb, could you try this out too?

> ---
> 
> This is an alternative to the patch by Michael S. Tsirkin
> titled "virtio-pci: add softlinks between virtio and pci"
> https://patchwork.kernel.org/patch/454581/
> 
> It creates simpler code, uses less memory, and should
> be even easier use by the installer as it won't have to
> know a virtio symlink to follow (just follow none).
> 
> Compile tested only as I don't have kvm setup.
> 
> 
> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> index ef8d9d5..4fb5b2b 100644
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -96,11 +96,6 @@ static struct pci_device_id virtio_pci_id_table[] = {
>  
>  MODULE_DEVICE_TABLE(pci, virtio_pci_id_table);
>  
> -/* A PCI device has it's own struct device and so does a virtio device so
> - * we create a place for the virtio devices to show up in sysfs.  I think it
> - * would make more sense for virtio to not insist on having it's own device. */
> -static struct device *virtio_pci_root;
> -
>  /* Convert a generic virtio device to our structure */
>  static struct virtio_pci_device *to_vp_device(struct virtio_device *vdev)
>  {
> @@ -629,7 +624,7 @@ static int __devinit virtio_pci_probe(struct pci_dev *pci_dev,
>  	if (vp_dev == NULL)
>  		return -ENOMEM;
>  
> -	vp_dev->vdev.dev.parent = virtio_pci_root;
> +	vp_dev->vdev.dev.parent = &pci_dev->dev;
>  	vp_dev->vdev.dev.release = virtio_pci_release_dev;
>  	vp_dev->vdev.config = &virtio_pci_config_ops;
>  	vp_dev->pci_dev = pci_dev;
> @@ -717,17 +712,7 @@ static struct pci_driver virtio_pci_driver = {
>  
>  static int __init virtio_pci_init(void)
>  {
> -	int err;
> -
> -	virtio_pci_root = root_device_register("virtio-pci");
> -	if (IS_ERR(virtio_pci_root))
> -		return PTR_ERR(virtio_pci_root);
> -
> -	err = pci_register_driver(&virtio_pci_driver);
> -	if (err)
> -		root_device_unregister(virtio_pci_root);
> -
> -	return err;
> +	return pci_register_driver(&virtio_pci_driver);
>  }
>  
>  module_init(virtio_pci_init);
> @@ -735,7 +720,6 @@ module_init(virtio_pci_init);
>  static void __exit virtio_pci_exit(void)
>  {
>  	pci_unregister_driver(&virtio_pci_driver);
> -	root_device_unregister(virtio_pci_root);
>  }
>  
>  module_exit(virtio_pci_exit);

Re: [PATCH] virtio: remove virtio-pci root device

[Gleb Natapov]

On Sun, Jan 09, 2011 at 05:18:21PM +0200, Michael S. Tsirkin wrote:
> On Fri, Jan 07, 2011 at 02:55:06AM -0600, Milton Miller wrote:
> > We sometimes need to map between the virtio device and
> > the given pci device. One such use is OS installer that
> > gets the boot pci device from BIOS and needs to
> > find the relevant block device. Since it can't,
> > installation fails.
> > 
> > Instead of creating a top-level devices/virtio-pci
> > directory, create each device under the corresponding
> > pci device node.  Symlinks to all virtio-pci
> > devices can be found under the pci driver link in
> > bus/pci/drivers/virtio-pci/devices, and all virtio
> > devices under drivers/bus/virtio/devices.
> > 
> > Signed-off-by: Milton Miller <miltonm@bga.com>
> 
> OK, this works fine for me.  I played with options to add compat
> softlinks under devices/virtio-pci but we still don't get exactly the
> same layout and since I don't think anyone actually uses them, it's
> probably ok to just to the simple thing.
> 
> Tested/Acked-by: Michael S. Tsirkin <mst@redhat.com>
> 
> Rusty, since this help fix at least one user, any chance this can be put
> in 2.6.38? OK to backport to -stable?
> 
> Gleb, could you try this out too?
> 
With this patch if I have 3 virtio disks for a VM I get:
/sys/devices/pci0000:00/0000:00:04.0/virtio0/block/vda
/sys/devices/pci0000:00/0000:00:05.0/virtio1/block/vdb
/sys/devices/pci0000:00/0000:00:06.0/virtio2/block/vdc

Number after virtio has no much sense. It either should be dropped at all
or be always zero in case we will support more then one virtio controller
per pci card. In that case each virtio controller will have directories
virtio0/virtio1/virtio2... under same pci device directory.


> > ---
> > 
> > This is an alternative to the patch by Michael S. Tsirkin
> > titled "virtio-pci: add softlinks between virtio and pci"
> > https://patchwork.kernel.org/patch/454581/
> > 
> > It creates simpler code, uses less memory, and should
> > be even easier use by the installer as it won't have to
> > know a virtio symlink to follow (just follow none).
> > 
> > Compile tested only as I don't have kvm setup.
> > 
> > 
> > diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> > index ef8d9d5..4fb5b2b 100644
> > --- a/drivers/virtio/virtio_pci.c
> > +++ b/drivers/virtio/virtio_pci.c
> > @@ -96,11 +96,6 @@ static struct pci_device_id virtio_pci_id_table[] = {
> >  
> >  MODULE_DEVICE_TABLE(pci, virtio_pci_id_table);
> >  
> > -/* A PCI device has it's own struct device and so does a virtio device so
> > - * we create a place for the virtio devices to show up in sysfs.  I think it
> > - * would make more sense for virtio to not insist on having it's own device. */
> > -static struct device *virtio_pci_root;
> > -
> >  /* Convert a generic virtio device to our structure */
> >  static struct virtio_pci_device *to_vp_device(struct virtio_device *vdev)
> >  {
> > @@ -629,7 +624,7 @@ static int __devinit virtio_pci_probe(struct pci_dev *pci_dev,
> >  	if (vp_dev == NULL)
> >  		return -ENOMEM;
> >  
> > -	vp_dev->vdev.dev.parent = virtio_pci_root;
> > +	vp_dev->vdev.dev.parent = &pci_dev->dev;
> >  	vp_dev->vdev.dev.release = virtio_pci_release_dev;
> >  	vp_dev->vdev.config = &virtio_pci_config_ops;
> >  	vp_dev->pci_dev = pci_dev;
> > @@ -717,17 +712,7 @@ static struct pci_driver virtio_pci_driver = {
> >  
> >  static int __init virtio_pci_init(void)
> >  {
> > -	int err;
> > -
> > -	virtio_pci_root = root_device_register("virtio-pci");
> > -	if (IS_ERR(virtio_pci_root))
> > -		return PTR_ERR(virtio_pci_root);
> > -
> > -	err = pci_register_driver(&virtio_pci_driver);
> > -	if (err)
> > -		root_device_unregister(virtio_pci_root);
> > -
> > -	return err;
> > +	return pci_register_driver(&virtio_pci_driver);
> >  }
> >  
> >  module_init(virtio_pci_init);
> > @@ -735,7 +720,6 @@ module_init(virtio_pci_init);
> >  static void __exit virtio_pci_exit(void)
> >  {
> >  	pci_unregister_driver(&virtio_pci_driver);
> > -	root_device_unregister(virtio_pci_root);
> >  }
> >  
> >  module_exit(virtio_pci_exit);

--
			Gleb.

Re: [PATCH] virtio: remove virtio-pci root device

[Michael S. Tsirkin]

On Mon, Jan 10, 2011 at 01:18:59PM +0200, Gleb Natapov wrote:
> On Sun, Jan 09, 2011 at 05:18:21PM +0200, Michael S. Tsirkin wrote:
> > On Fri, Jan 07, 2011 at 02:55:06AM -0600, Milton Miller wrote:
> > > We sometimes need to map between the virtio device and
> > > the given pci device. One such use is OS installer that
> > > gets the boot pci device from BIOS and needs to
> > > find the relevant block device. Since it can't,
> > > installation fails.
> > > 
> > > Instead of creating a top-level devices/virtio-pci
> > > directory, create each device under the corresponding
> > > pci device node.  Symlinks to all virtio-pci
> > > devices can be found under the pci driver link in
> > > bus/pci/drivers/virtio-pci/devices, and all virtio
> > > devices under drivers/bus/virtio/devices.
> > > 
> > > Signed-off-by: Milton Miller <miltonm@bga.com>
> > 
> > OK, this works fine for me.  I played with options to add compat
> > softlinks under devices/virtio-pci but we still don't get exactly the
> > same layout and since I don't think anyone actually uses them, it's
> > probably ok to just to the simple thing.
> > 
> > Tested/Acked-by: Michael S. Tsirkin <mst@redhat.com>
> > 
> > Rusty, since this help fix at least one user, any chance this can be put
> > in 2.6.38? OK to backport to -stable?
> > 
> > Gleb, could you try this out too?
> > 
> With this patch if I have 3 virtio disks for a VM I get:
> /sys/devices/pci0000:00/0000:00:04.0/virtio0/block/vda
> /sys/devices/pci0000:00/0000:00:05.0/virtio1/block/vdb
> /sys/devices/pci0000:00/0000:00:06.0/virtio2/block/vdc
> 
> Number after virtio has no much sense. It either should be dropped at all
> or be always zero in case we will support more then one virtio controller
> per pci card. In that case each virtio controller will have directories
> virtio0/virtio1/virtio2... under same pci device directory.

Yes. But this is the bus name.  It must be unique - all devices
also appear under /sys/bus/virtio/devices.

> > > ---
> > > 
> > > This is an alternative to the patch by Michael S. Tsirkin
> > > titled "virtio-pci: add softlinks between virtio and pci"
> > > https://patchwork.kernel.org/patch/454581/
> > > 
> > > It creates simpler code, uses less memory, and should
> > > be even easier use by the installer as it won't have to
> > > know a virtio symlink to follow (just follow none).
> > > 
> > > Compile tested only as I don't have kvm setup.
> > > 
> > > 
> > > diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> > > index ef8d9d5..4fb5b2b 100644
> > > --- a/drivers/virtio/virtio_pci.c
> > > +++ b/drivers/virtio/virtio_pci.c
> > > @@ -96,11 +96,6 @@ static struct pci_device_id virtio_pci_id_table[] = {
> > >  
> > >  MODULE_DEVICE_TABLE(pci, virtio_pci_id_table);
> > >  
> > > -/* A PCI device has it's own struct device and so does a virtio device so
> > > - * we create a place for the virtio devices to show up in sysfs.  I think it
> > > - * would make more sense for virtio to not insist on having it's own device. */
> > > -static struct device *virtio_pci_root;
> > > -
> > >  /* Convert a generic virtio device to our structure */
> > >  static struct virtio_pci_device *to_vp_device(struct virtio_device *vdev)
> > >  {
> > > @@ -629,7 +624,7 @@ static int __devinit virtio_pci_probe(struct pci_dev *pci_dev,
> > >  	if (vp_dev == NULL)
> > >  		return -ENOMEM;
> > >  
> > > -	vp_dev->vdev.dev.parent = virtio_pci_root;
> > > +	vp_dev->vdev.dev.parent = &pci_dev->dev;
> > >  	vp_dev->vdev.dev.release = virtio_pci_release_dev;
> > >  	vp_dev->vdev.config = &virtio_pci_config_ops;
> > >  	vp_dev->pci_dev = pci_dev;
> > > @@ -717,17 +712,7 @@ static struct pci_driver virtio_pci_driver = {
> > >  
> > >  static int __init virtio_pci_init(void)
> > >  {
> > > -	int err;
> > > -
> > > -	virtio_pci_root = root_device_register("virtio-pci");
> > > -	if (IS_ERR(virtio_pci_root))
> > > -		return PTR_ERR(virtio_pci_root);
> > > -
> > > -	err = pci_register_driver(&virtio_pci_driver);
> > > -	if (err)
> > > -		root_device_unregister(virtio_pci_root);
> > > -
> > > -	return err;
> > > +	return pci_register_driver(&virtio_pci_driver);
> > >  }
> > >  
> > >  module_init(virtio_pci_init);
> > > @@ -735,7 +720,6 @@ module_init(virtio_pci_init);
> > >  static void __exit virtio_pci_exit(void)
> > >  {
> > >  	pci_unregister_driver(&virtio_pci_driver);
> > > -	root_device_unregister(virtio_pci_root);
> > >  }
> > >  
> > >  module_exit(virtio_pci_exit);
> 
> --
> 			Gleb.

Re: [PATCH] virtio: remove virtio-pci root device

[Gleb Natapov]

On Mon, Jan 10, 2011 at 01:22:05PM +0200, Michael S. Tsirkin wrote:
> On Mon, Jan 10, 2011 at 01:18:59PM +0200, Gleb Natapov wrote:
> > On Sun, Jan 09, 2011 at 05:18:21PM +0200, Michael S. Tsirkin wrote:
> > > On Fri, Jan 07, 2011 at 02:55:06AM -0600, Milton Miller wrote:
> > > > We sometimes need to map between the virtio device and
> > > > the given pci device. One such use is OS installer that
> > > > gets the boot pci device from BIOS and needs to
> > > > find the relevant block device. Since it can't,
> > > > installation fails.
> > > > 
> > > > Instead of creating a top-level devices/virtio-pci
> > > > directory, create each device under the corresponding
> > > > pci device node.  Symlinks to all virtio-pci
> > > > devices can be found under the pci driver link in
> > > > bus/pci/drivers/virtio-pci/devices, and all virtio
> > > > devices under drivers/bus/virtio/devices.
> > > > 
> > > > Signed-off-by: Milton Miller <miltonm@bga.com>
> > > 
> > > OK, this works fine for me.  I played with options to add compat
> > > softlinks under devices/virtio-pci but we still don't get exactly the
> > > same layout and since I don't think anyone actually uses them, it's
> > > probably ok to just to the simple thing.
> > > 
> > > Tested/Acked-by: Michael S. Tsirkin <mst@redhat.com>
> > > 
> > > Rusty, since this help fix at least one user, any chance this can be put
> > > in 2.6.38? OK to backport to -stable?
> > > 
> > > Gleb, could you try this out too?
> > > 
> > With this patch if I have 3 virtio disks for a VM I get:
> > /sys/devices/pci0000:00/0000:00:04.0/virtio0/block/vda
> > /sys/devices/pci0000:00/0000:00:05.0/virtio1/block/vdb
> > /sys/devices/pci0000:00/0000:00:06.0/virtio2/block/vdc
> > 
> > Number after virtio has no much sense. It either should be dropped at all
> > or be always zero in case we will support more then one virtio controller
> > per pci card. In that case each virtio controller will have directories
> > virtio0/virtio1/virtio2... under same pci device directory.
> 
> Yes. But this is the bus name.  It must be unique - all devices
> also appear under /sys/bus/virtio/devices.
> 
It is very strange king of bus that is spread over several PCI devices :)
It doesn't make much sense IMHO, but I can leave with it.

> > > > ---
> > > > 
> > > > This is an alternative to the patch by Michael S. Tsirkin
> > > > titled "virtio-pci: add softlinks between virtio and pci"
> > > > https://patchwork.kernel.org/patch/454581/
> > > > 
> > > > It creates simpler code, uses less memory, and should
> > > > be even easier use by the installer as it won't have to
> > > > know a virtio symlink to follow (just follow none).
> > > > 
> > > > Compile tested only as I don't have kvm setup.
> > > > 
> > > > 
> > > > diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> > > > index ef8d9d5..4fb5b2b 100644
> > > > --- a/drivers/virtio/virtio_pci.c
> > > > +++ b/drivers/virtio/virtio_pci.c
> > > > @@ -96,11 +96,6 @@ static struct pci_device_id virtio_pci_id_table[] = {
> > > >  
> > > >  MODULE_DEVICE_TABLE(pci, virtio_pci_id_table);
> > > >  
> > > > -/* A PCI device has it's own struct device and so does a virtio device so
> > > > - * we create a place for the virtio devices to show up in sysfs.  I think it
> > > > - * would make more sense for virtio to not insist on having it's own device. */
> > > > -static struct device *virtio_pci_root;
> > > > -
> > > >  /* Convert a generic virtio device to our structure */
> > > >  static struct virtio_pci_device *to_vp_device(struct virtio_device *vdev)
> > > >  {
> > > > @@ -629,7 +624,7 @@ static int __devinit virtio_pci_probe(struct pci_dev *pci_dev,
> > > >  	if (vp_dev == NULL)
> > > >  		return -ENOMEM;
> > > >  
> > > > -	vp_dev->vdev.dev.parent = virtio_pci_root;
> > > > +	vp_dev->vdev.dev.parent = &pci_dev->dev;
> > > >  	vp_dev->vdev.dev.release = virtio_pci_release_dev;
> > > >  	vp_dev->vdev.config = &virtio_pci_config_ops;
> > > >  	vp_dev->pci_dev = pci_dev;
> > > > @@ -717,17 +712,7 @@ static struct pci_driver virtio_pci_driver = {
> > > >  
> > > >  static int __init virtio_pci_init(void)
> > > >  {
> > > > -	int err;
> > > > -
> > > > -	virtio_pci_root = root_device_register("virtio-pci");
> > > > -	if (IS_ERR(virtio_pci_root))
> > > > -		return PTR_ERR(virtio_pci_root);
> > > > -
> > > > -	err = pci_register_driver(&virtio_pci_driver);
> > > > -	if (err)
> > > > -		root_device_unregister(virtio_pci_root);
> > > > -
> > > > -	return err;
> > > > +	return pci_register_driver(&virtio_pci_driver);
> > > >  }
> > > >  
> > > >  module_init(virtio_pci_init);
> > > > @@ -735,7 +720,6 @@ module_init(virtio_pci_init);
> > > >  static void __exit virtio_pci_exit(void)
> > > >  {
> > > >  	pci_unregister_driver(&virtio_pci_driver);
> > > > -	root_device_unregister(virtio_pci_root);
> > > >  }
> > > >  
> > > >  module_exit(virtio_pci_exit);
> > 
> > --
> > 			Gleb.

--
			Gleb.

Re: [PATCH] virtio: remove virtio-pci root device

[Gleb Natapov]

On Mon, Jan 10, 2011 at 01:27:39PM +0200, Gleb Natapov wrote:
> On Mon, Jan 10, 2011 at 01:22:05PM +0200, Michael S. Tsirkin wrote:
> > On Mon, Jan 10, 2011 at 01:18:59PM +0200, Gleb Natapov wrote:
> > > On Sun, Jan 09, 2011 at 05:18:21PM +0200, Michael S. Tsirkin wrote:
> > > > On Fri, Jan 07, 2011 at 02:55:06AM -0600, Milton Miller wrote:
> > > > > We sometimes need to map between the virtio device and
> > > > > the given pci device. One such use is OS installer that
> > > > > gets the boot pci device from BIOS and needs to
> > > > > find the relevant block device. Since it can't,
> > > > > installation fails.
> > > > > 
> > > > > Instead of creating a top-level devices/virtio-pci
> > > > > directory, create each device under the corresponding
> > > > > pci device node.  Symlinks to all virtio-pci
> > > > > devices can be found under the pci driver link in
> > > > > bus/pci/drivers/virtio-pci/devices, and all virtio
> > > > > devices under drivers/bus/virtio/devices.
> > > > > 
> > > > > Signed-off-by: Milton Miller <miltonm@bga.com>
> > > > 
> > > > OK, this works fine for me.  I played with options to add compat
> > > > softlinks under devices/virtio-pci but we still don't get exactly the
> > > > same layout and since I don't think anyone actually uses them, it's
> > > > probably ok to just to the simple thing.
> > > > 
> > > > Tested/Acked-by: Michael S. Tsirkin <mst@redhat.com>
> > > > 
> > > > Rusty, since this help fix at least one user, any chance this can be put
> > > > in 2.6.38? OK to backport to -stable?
> > > > 
> > > > Gleb, could you try this out too?
> > > > 
> > > With this patch if I have 3 virtio disks for a VM I get:
> > > /sys/devices/pci0000:00/0000:00:04.0/virtio0/block/vda
> > > /sys/devices/pci0000:00/0000:00:05.0/virtio1/block/vdb
> > > /sys/devices/pci0000:00/0000:00:06.0/virtio2/block/vdc
> > > 
> > > Number after virtio has no much sense. It either should be dropped at all
> > > or be always zero in case we will support more then one virtio controller
> > > per pci card. In that case each virtio controller will have directories
> > > virtio0/virtio1/virtio2... under same pci device directory.
> > 
> > Yes. But this is the bus name.  It must be unique - all devices
> > also appear under /sys/bus/virtio/devices.
> > 
> It is very strange king of bus that is spread over several PCI devices :)
> It doesn't make much sense IMHO, but I can leave with it.
> 
I can't "leave" with it, but I can "live" with it.

--
			Gleb.

Re: [PATCH] virtio: remove virtio-pci root device

[Michael S. Tsirkin]

On Mon, Jan 10, 2011 at 01:42:19PM +0200, Gleb Natapov wrote:
> On Mon, Jan 10, 2011 at 01:27:39PM +0200, Gleb Natapov wrote:
> > On Mon, Jan 10, 2011 at 01:22:05PM +0200, Michael S. Tsirkin wrote:
> > > On Mon, Jan 10, 2011 at 01:18:59PM +0200, Gleb Natapov wrote:
> > > > On Sun, Jan 09, 2011 at 05:18:21PM +0200, Michael S. Tsirkin wrote:
> > > > > On Fri, Jan 07, 2011 at 02:55:06AM -0600, Milton Miller wrote:
> > > > > > We sometimes need to map between the virtio device and
> > > > > > the given pci device. One such use is OS installer that
> > > > > > gets the boot pci device from BIOS and needs to
> > > > > > find the relevant block device. Since it can't,
> > > > > > installation fails.
> > > > > > 
> > > > > > Instead of creating a top-level devices/virtio-pci
> > > > > > directory, create each device under the corresponding
> > > > > > pci device node.  Symlinks to all virtio-pci
> > > > > > devices can be found under the pci driver link in
> > > > > > bus/pci/drivers/virtio-pci/devices, and all virtio
> > > > > > devices under drivers/bus/virtio/devices.
> > > > > > 
> > > > > > Signed-off-by: Milton Miller <miltonm@bga.com>
> > > > > 
> > > > > OK, this works fine for me.  I played with options to add compat
> > > > > softlinks under devices/virtio-pci but we still don't get exactly the
> > > > > same layout and since I don't think anyone actually uses them, it's
> > > > > probably ok to just to the simple thing.
> > > > > 
> > > > > Tested/Acked-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > 
> > > > > Rusty, since this help fix at least one user, any chance this can be put
> > > > > in 2.6.38? OK to backport to -stable?
> > > > > 
> > > > > Gleb, could you try this out too?
> > > > > 
> > > > With this patch if I have 3 virtio disks for a VM I get:
> > > > /sys/devices/pci0000:00/0000:00:04.0/virtio0/block/vda
> > > > /sys/devices/pci0000:00/0000:00:05.0/virtio1/block/vdb
> > > > /sys/devices/pci0000:00/0000:00:06.0/virtio2/block/vdc
> > > > 
> > > > Number after virtio has no much sense. It either should be dropped at all
> > > > or be always zero in case we will support more then one virtio controller
> > > > per pci card. In that case each virtio controller will have directories
> > > > virtio0/virtio1/virtio2... under same pci device directory.
> > > 
> > > Yes. But this is the bus name.  It must be unique - all devices
> > > also appear under /sys/bus/virtio/devices.
> > > 
> > It is very strange king of bus that is spread over several PCI devices :)
> > It doesn't make much sense IMHO, but I can leave with it.
> > 
> I can't "leave" with it, but I can "live" with it.

The virtio bus is an attempt to make as many applications as
possible work transparently on any virtio system,
be it lguest, s390 or pci. Arbitrary IDs is just a hint to the
applications 'don't rely on the name at all'.

> --
> 			Gleb.

Re: [PATCH] virtio: remove virtio-pci root device

[Gleb Natapov]

On Mon, Jan 10, 2011 at 02:08:08PM +0200, Michael S. Tsirkin wrote:
> On Mon, Jan 10, 2011 at 01:42:19PM +0200, Gleb Natapov wrote:
> > On Mon, Jan 10, 2011 at 01:27:39PM +0200, Gleb Natapov wrote:
> > > On Mon, Jan 10, 2011 at 01:22:05PM +0200, Michael S. Tsirkin wrote:
> > > > On Mon, Jan 10, 2011 at 01:18:59PM +0200, Gleb Natapov wrote:
> > > > > On Sun, Jan 09, 2011 at 05:18:21PM +0200, Michael S. Tsirkin wrote:
> > > > > > On Fri, Jan 07, 2011 at 02:55:06AM -0600, Milton Miller wrote:
> > > > > > > We sometimes need to map between the virtio device and
> > > > > > > the given pci device. One such use is OS installer that
> > > > > > > gets the boot pci device from BIOS and needs to
> > > > > > > find the relevant block device. Since it can't,
> > > > > > > installation fails.
> > > > > > > 
> > > > > > > Instead of creating a top-level devices/virtio-pci
> > > > > > > directory, create each device under the corresponding
> > > > > > > pci device node.  Symlinks to all virtio-pci
> > > > > > > devices can be found under the pci driver link in
> > > > > > > bus/pci/drivers/virtio-pci/devices, and all virtio
> > > > > > > devices under drivers/bus/virtio/devices.
> > > > > > > 
> > > > > > > Signed-off-by: Milton Miller <miltonm@bga.com>
> > > > > > 
> > > > > > OK, this works fine for me.  I played with options to add compat
> > > > > > softlinks under devices/virtio-pci but we still don't get exactly the
> > > > > > same layout and since I don't think anyone actually uses them, it's
> > > > > > probably ok to just to the simple thing.
> > > > > > 
> > > > > > Tested/Acked-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > > 
> > > > > > Rusty, since this help fix at least one user, any chance this can be put
> > > > > > in 2.6.38? OK to backport to -stable?
> > > > > > 
> > > > > > Gleb, could you try this out too?
> > > > > > 
> > > > > With this patch if I have 3 virtio disks for a VM I get:
> > > > > /sys/devices/pci0000:00/0000:00:04.0/virtio0/block/vda
> > > > > /sys/devices/pci0000:00/0000:00:05.0/virtio1/block/vdb
> > > > > /sys/devices/pci0000:00/0000:00:06.0/virtio2/block/vdc
> > > > > 
> > > > > Number after virtio has no much sense. It either should be dropped at all
> > > > > or be always zero in case we will support more then one virtio controller
> > > > > per pci card. In that case each virtio controller will have directories
> > > > > virtio0/virtio1/virtio2... under same pci device directory.
> > > > 
> > > > Yes. But this is the bus name.  It must be unique - all devices
> > > > also appear under /sys/bus/virtio/devices.
> > > > 
> > > It is very strange king of bus that is spread over several PCI devices :)
> > > It doesn't make much sense IMHO, but I can leave with it.
> > > 
> > I can't "leave" with it, but I can "live" with it.
> 
> The virtio bus is an attempt to make as many applications as
> possible work transparently on any virtio system,
> be it lguest, s390 or pci. Arbitrary IDs is just a hint to the
> applications 'don't rely on the name at all'.
> 
Shouldn't sysfs directory reflect real device topology? There are other
buses AFAIK that connected differently on different HW. How virtio is
special? What applications this allow to work transparently?

--
			Gleb.

Re: [PATCH] virtio: remove virtio-pci root device

[Michael S. Tsirkin]

On Mon, Jan 10, 2011 at 02:50:11PM +0200, Gleb Natapov wrote:
> On Mon, Jan 10, 2011 at 02:08:08PM +0200, Michael S. Tsirkin wrote:
> > On Mon, Jan 10, 2011 at 01:42:19PM +0200, Gleb Natapov wrote:
> > > On Mon, Jan 10, 2011 at 01:27:39PM +0200, Gleb Natapov wrote:
> > > > On Mon, Jan 10, 2011 at 01:22:05PM +0200, Michael S. Tsirkin wrote:
> > > > > On Mon, Jan 10, 2011 at 01:18:59PM +0200, Gleb Natapov wrote:
> > > > > > On Sun, Jan 09, 2011 at 05:18:21PM +0200, Michael S. Tsirkin wrote:
> > > > > > > On Fri, Jan 07, 2011 at 02:55:06AM -0600, Milton Miller wrote:
> > > > > > > > We sometimes need to map between the virtio device and
> > > > > > > > the given pci device. One such use is OS installer that
> > > > > > > > gets the boot pci device from BIOS and needs to
> > > > > > > > find the relevant block device. Since it can't,
> > > > > > > > installation fails.
> > > > > > > > 
> > > > > > > > Instead of creating a top-level devices/virtio-pci
> > > > > > > > directory, create each device under the corresponding
> > > > > > > > pci device node.  Symlinks to all virtio-pci
> > > > > > > > devices can be found under the pci driver link in
> > > > > > > > bus/pci/drivers/virtio-pci/devices, and all virtio
> > > > > > > > devices under drivers/bus/virtio/devices.
> > > > > > > > 
> > > > > > > > Signed-off-by: Milton Miller <miltonm@bga.com>
> > > > > > > 
> > > > > > > OK, this works fine for me.  I played with options to add compat
> > > > > > > softlinks under devices/virtio-pci but we still don't get exactly the
> > > > > > > same layout and since I don't think anyone actually uses them, it's
> > > > > > > probably ok to just to the simple thing.
> > > > > > > 
> > > > > > > Tested/Acked-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > > > 
> > > > > > > Rusty, since this help fix at least one user, any chance this can be put
> > > > > > > in 2.6.38? OK to backport to -stable?
> > > > > > > 
> > > > > > > Gleb, could you try this out too?
> > > > > > > 
> > > > > > With this patch if I have 3 virtio disks for a VM I get:
> > > > > > /sys/devices/pci0000:00/0000:00:04.0/virtio0/block/vda
> > > > > > /sys/devices/pci0000:00/0000:00:05.0/virtio1/block/vdb
> > > > > > /sys/devices/pci0000:00/0000:00:06.0/virtio2/block/vdc
> > > > > > 
> > > > > > Number after virtio has no much sense. It either should be dropped at all
> > > > > > or be always zero in case we will support more then one virtio controller
> > > > > > per pci card. In that case each virtio controller will have directories
> > > > > > virtio0/virtio1/virtio2... under same pci device directory.
> > > > > 
> > > > > Yes. But this is the bus name.  It must be unique - all devices
> > > > > also appear under /sys/bus/virtio/devices.
> > > > > 
> > > > It is very strange king of bus that is spread over several PCI devices :)
> > > > It doesn't make much sense IMHO, but I can leave with it.
> > > > 
> > > I can't "leave" with it, but I can "live" with it.
> > 
> > The virtio bus is an attempt to make as many applications as
> > possible work transparently on any virtio system,
> > be it lguest, s390 or pci. Arbitrary IDs is just a hint to the
> > applications 'don't rely on the name at all'.
> > 
> Shouldn't sysfs directory reflect real device topology?

What exactly is proposed here? Are you acking or nacking this patch?
Prefer the older version? Can send your own?

> There are other buses AFAIK that connected differently on different
> HW. How virtio is special?
> What applications this allow to work transparently?

I don't know which applications use the current sysfs topology
but the assumption that there are none and we can break it
arbitrarily seems a bit drastic.

> --
> 			Gleb.

Re: [PATCH] virtio: remove virtio-pci root device

[Gleb Natapov]

On Mon, Jan 10, 2011 at 03:31:40PM +0200, Michael S. Tsirkin wrote:
> On Mon, Jan 10, 2011 at 02:50:11PM +0200, Gleb Natapov wrote:
> > On Mon, Jan 10, 2011 at 02:08:08PM +0200, Michael S. Tsirkin wrote:
> > > On Mon, Jan 10, 2011 at 01:42:19PM +0200, Gleb Natapov wrote:
> > > > On Mon, Jan 10, 2011 at 01:27:39PM +0200, Gleb Natapov wrote:
> > > > > On Mon, Jan 10, 2011 at 01:22:05PM +0200, Michael S. Tsirkin wrote:
> > > > > > On Mon, Jan 10, 2011 at 01:18:59PM +0200, Gleb Natapov wrote:
> > > > > > > On Sun, Jan 09, 2011 at 05:18:21PM +0200, Michael S. Tsirkin wrote:
> > > > > > > > On Fri, Jan 07, 2011 at 02:55:06AM -0600, Milton Miller wrote:
> > > > > > > > > We sometimes need to map between the virtio device and
> > > > > > > > > the given pci device. One such use is OS installer that
> > > > > > > > > gets the boot pci device from BIOS and needs to
> > > > > > > > > find the relevant block device. Since it can't,
> > > > > > > > > installation fails.
> > > > > > > > > 
> > > > > > > > > Instead of creating a top-level devices/virtio-pci
> > > > > > > > > directory, create each device under the corresponding
> > > > > > > > > pci device node.  Symlinks to all virtio-pci
> > > > > > > > > devices can be found under the pci driver link in
> > > > > > > > > bus/pci/drivers/virtio-pci/devices, and all virtio
> > > > > > > > > devices under drivers/bus/virtio/devices.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Milton Miller <miltonm@bga.com>
> > > > > > > > 
> > > > > > > > OK, this works fine for me.  I played with options to add compat
> > > > > > > > softlinks under devices/virtio-pci but we still don't get exactly the
> > > > > > > > same layout and since I don't think anyone actually uses them, it's
> > > > > > > > probably ok to just to the simple thing.
> > > > > > > > 
> > > > > > > > Tested/Acked-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > > > > 
> > > > > > > > Rusty, since this help fix at least one user, any chance this can be put
> > > > > > > > in 2.6.38? OK to backport to -stable?
> > > > > > > > 
> > > > > > > > Gleb, could you try this out too?
> > > > > > > > 
> > > > > > > With this patch if I have 3 virtio disks for a VM I get:
> > > > > > > /sys/devices/pci0000:00/0000:00:04.0/virtio0/block/vda
> > > > > > > /sys/devices/pci0000:00/0000:00:05.0/virtio1/block/vdb
> > > > > > > /sys/devices/pci0000:00/0000:00:06.0/virtio2/block/vdc
> > > > > > > 
> > > > > > > Number after virtio has no much sense. It either should be dropped at all
> > > > > > > or be always zero in case we will support more then one virtio controller
> > > > > > > per pci card. In that case each virtio controller will have directories
> > > > > > > virtio0/virtio1/virtio2... under same pci device directory.
> > > > > > 
> > > > > > Yes. But this is the bus name.  It must be unique - all devices
> > > > > > also appear under /sys/bus/virtio/devices.
> > > > > > 
> > > > > It is very strange king of bus that is spread over several PCI devices :)
> > > > > It doesn't make much sense IMHO, but I can leave with it.
> > > > > 
> > > > I can't "leave" with it, but I can "live" with it.
> > > 
> > > The virtio bus is an attempt to make as many applications as
> > > possible work transparently on any virtio system,
> > > be it lguest, s390 or pci. Arbitrary IDs is just a hint to the
> > > applications 'don't rely on the name at all'.
> > > 
> > Shouldn't sysfs directory reflect real device topology?
> 
> What exactly is proposed here? Are you acking or nacking this patch?
> Prefer the older version? Can send your own?
> 
What exactly is proposed is to get rid of random number in virtio
directory name. If this is complicated, as I said above, I can live
with it as is.

> > There are other buses AFAIK that connected differently on different
> > HW. How virtio is special?
> > What applications this allow to work transparently?
> 
> I don't know which applications use the current sysfs topology
> but the assumption that there are none and we can break it
> arbitrarily seems a bit drastic.
> 
> > --
> > 			Gleb.

--
			Gleb.

Re: [PATCH] virtio: remove virtio-pci root device

[Michael S. Tsirkin]

On Mon, Jan 10, 2011 at 03:40:50PM +0200, Gleb Natapov wrote:
> On Mon, Jan 10, 2011 at 03:31:40PM +0200, Michael S. Tsirkin wrote:
> > On Mon, Jan 10, 2011 at 02:50:11PM +0200, Gleb Natapov wrote:
> > > On Mon, Jan 10, 2011 at 02:08:08PM +0200, Michael S. Tsirkin wrote:
> > > > On Mon, Jan 10, 2011 at 01:42:19PM +0200, Gleb Natapov wrote:
> > > > > On Mon, Jan 10, 2011 at 01:27:39PM +0200, Gleb Natapov wrote:
> > > > > > On Mon, Jan 10, 2011 at 01:22:05PM +0200, Michael S. Tsirkin wrote:
> > > > > > > On Mon, Jan 10, 2011 at 01:18:59PM +0200, Gleb Natapov wrote:
> > > > > > > > On Sun, Jan 09, 2011 at 05:18:21PM +0200, Michael S. Tsirkin wrote:
> > > > > > > > > On Fri, Jan 07, 2011 at 02:55:06AM -0600, Milton Miller wrote:
> > > > > > > > > > We sometimes need to map between the virtio device and
> > > > > > > > > > the given pci device. One such use is OS installer that
> > > > > > > > > > gets the boot pci device from BIOS and needs to
> > > > > > > > > > find the relevant block device. Since it can't,
> > > > > > > > > > installation fails.
> > > > > > > > > > 
> > > > > > > > > > Instead of creating a top-level devices/virtio-pci
> > > > > > > > > > directory, create each device under the corresponding
> > > > > > > > > > pci device node.  Symlinks to all virtio-pci
> > > > > > > > > > devices can be found under the pci driver link in
> > > > > > > > > > bus/pci/drivers/virtio-pci/devices, and all virtio
> > > > > > > > > > devices under drivers/bus/virtio/devices.
> > > > > > > > > > 
> > > > > > > > > > Signed-off-by: Milton Miller <miltonm@bga.com>
> > > > > > > > > 
> > > > > > > > > OK, this works fine for me.  I played with options to add compat
> > > > > > > > > softlinks under devices/virtio-pci but we still don't get exactly the
> > > > > > > > > same layout and since I don't think anyone actually uses them, it's
> > > > > > > > > probably ok to just to the simple thing.
> > > > > > > > > 
> > > > > > > > > Tested/Acked-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > > > > > 
> > > > > > > > > Rusty, since this help fix at least one user, any chance this can be put
> > > > > > > > > in 2.6.38? OK to backport to -stable?
> > > > > > > > > 
> > > > > > > > > Gleb, could you try this out too?
> > > > > > > > > 
> > > > > > > > With this patch if I have 3 virtio disks for a VM I get:
> > > > > > > > /sys/devices/pci0000:00/0000:00:04.0/virtio0/block/vda
> > > > > > > > /sys/devices/pci0000:00/0000:00:05.0/virtio1/block/vdb
> > > > > > > > /sys/devices/pci0000:00/0000:00:06.0/virtio2/block/vdc
> > > > > > > > 
> > > > > > > > Number after virtio has no much sense. It either should be dropped at all
> > > > > > > > or be always zero in case we will support more then one virtio controller
> > > > > > > > per pci card. In that case each virtio controller will have directories
> > > > > > > > virtio0/virtio1/virtio2... under same pci device directory.
> > > > > > > 
> > > > > > > Yes. But this is the bus name.  It must be unique - all devices
> > > > > > > also appear under /sys/bus/virtio/devices.
> > > > > > > 
> > > > > > It is very strange king of bus that is spread over several PCI devices :)
> > > > > > It doesn't make much sense IMHO, but I can leave with it.
> > > > > > 
> > > > > I can't "leave" with it, but I can "live" with it.
> > > > 
> > > > The virtio bus is an attempt to make as many applications as
> > > > possible work transparently on any virtio system,
> > > > be it lguest, s390 or pci. Arbitrary IDs is just a hint to the
> > > > applications 'don't rely on the name at all'.
> > > > 
> > > Shouldn't sysfs directory reflect real device topology?
> > 
> > What exactly is proposed here? Are you acking or nacking this patch?
> > Prefer the older version? Can send your own?
> > 
> What exactly is proposed is to get rid of random number in virtio
> directory name. If this is complicated, as I said above, I can live
> with it as is.

Certainly more complicated than this patch:
core currently uses same dev_name everywhere.

> > > There are other buses AFAIK that connected differently on different
> > > HW. How virtio is special?
> > > What applications this allow to work transparently?
> > 
> > I don't know which applications use the current sysfs topology
> > but the assumption that there are none and we can break it
> > arbitrarily seems a bit drastic.
> > 
> > > --
> > > 			Gleb.
> 
> --
> 			Gleb.

Re: [PATCH] virtio: remove virtio-pci root device

[Gleb Natapov]

On Fri, Jan 07, 2011 at 02:55:06AM -0600, Milton Miller wrote:
> We sometimes need to map between the virtio device and
> the given pci device. One such use is OS installer that
> gets the boot pci device from BIOS and needs to
> find the relevant block device. Since it can't,
> installation fails.
> 
> Instead of creating a top-level devices/virtio-pci
> directory, create each device under the corresponding
> pci device node.  Symlinks to all virtio-pci
> devices can be found under the pci driver link in
> bus/pci/drivers/virtio-pci/devices, and all virtio
> devices under drivers/bus/virtio/devices.
> 
> Signed-off-by: Milton Miller <miltonm@bga.com>
Acked-by: Gleb Natapov <gleb@redhat.com>

> ---
> 
> This is an alternative to the patch by Michael S. Tsirkin
> titled "virtio-pci: add softlinks between virtio and pci"
> https://patchwork.kernel.org/patch/454581/
> 
> It creates simpler code, uses less memory, and should
> be even easier use by the installer as it won't have to
> know a virtio symlink to follow (just follow none).
> 
> Compile tested only as I don't have kvm setup.
> 
> 
> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> index ef8d9d5..4fb5b2b 100644
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -96,11 +96,6 @@ static struct pci_device_id virtio_pci_id_table[] = {
>  
>  MODULE_DEVICE_TABLE(pci, virtio_pci_id_table);
>  
> -/* A PCI device has it's own struct device and so does a virtio device so
> - * we create a place for the virtio devices to show up in sysfs.  I think it
> - * would make more sense for virtio to not insist on having it's own device. */
> -static struct device *virtio_pci_root;
> -
>  /* Convert a generic virtio device to our structure */
>  static struct virtio_pci_device *to_vp_device(struct virtio_device *vdev)
>  {
> @@ -629,7 +624,7 @@ static int __devinit virtio_pci_probe(struct pci_dev *pci_dev,
>  	if (vp_dev == NULL)
>  		return -ENOMEM;
>  
> -	vp_dev->vdev.dev.parent = virtio_pci_root;
> +	vp_dev->vdev.dev.parent = &pci_dev->dev;
>  	vp_dev->vdev.dev.release = virtio_pci_release_dev;
>  	vp_dev->vdev.config = &virtio_pci_config_ops;
>  	vp_dev->pci_dev = pci_dev;
> @@ -717,17 +712,7 @@ static struct pci_driver virtio_pci_driver = {
>  
>  static int __init virtio_pci_init(void)
>  {
> -	int err;
> -
> -	virtio_pci_root = root_device_register("virtio-pci");
> -	if (IS_ERR(virtio_pci_root))
> -		return PTR_ERR(virtio_pci_root);
> -
> -	err = pci_register_driver(&virtio_pci_driver);
> -	if (err)
> -		root_device_unregister(virtio_pci_root);
> -
> -	return err;
> +	return pci_register_driver(&virtio_pci_driver);
>  }
>  
>  module_init(virtio_pci_init);
> @@ -735,7 +720,6 @@ module_init(virtio_pci_init);
>  static void __exit virtio_pci_exit(void)
>  {
>  	pci_unregister_driver(&virtio_pci_driver);
> -	root_device_unregister(virtio_pci_root);
>  }
>  
>  module_exit(virtio_pci_exit);

--
			Gleb.

Re: [PATCH] virtio: remove virtio-pci root device

[Michael S. Tsirkin]

On Mon, Jan 10, 2011 at 07:10:46PM +0200, Gleb Natapov wrote:
> On Fri, Jan 07, 2011 at 02:55:06AM -0600, Milton Miller wrote:
> > We sometimes need to map between the virtio device and
> > the given pci device. One such use is OS installer that
> > gets the boot pci device from BIOS and needs to
> > find the relevant block device. Since it can't,
> > installation fails.
> > 
> > Instead of creating a top-level devices/virtio-pci
> > directory, create each device under the corresponding
> > pci device node.  Symlinks to all virtio-pci
> > devices can be found under the pci driver link in
> > bus/pci/drivers/virtio-pci/devices, and all virtio
> > devices under drivers/bus/virtio/devices.
> > 
> > Signed-off-by: Milton Miller <miltonm@bga.com>
> Acked-by: Gleb Natapov <gleb@redhat.com>

Rusty, any comments?
We are going to tell the installer guys that this
is the interface, would be nice to know that the
patch is queued before we do.


> > ---
> > 
> > This is an alternative to the patch by Michael S. Tsirkin
> > titled "virtio-pci: add softlinks between virtio and pci"
> > https://patchwork.kernel.org/patch/454581/
> > 
> > It creates simpler code, uses less memory, and should
> > be even easier use by the installer as it won't have to
> > know a virtio symlink to follow (just follow none).
> > 
> > Compile tested only as I don't have kvm setup.
> > 
> > 
> > diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> > index ef8d9d5..4fb5b2b 100644
> > --- a/drivers/virtio/virtio_pci.c
> > +++ b/drivers/virtio/virtio_pci.c
> > @@ -96,11 +96,6 @@ static struct pci_device_id virtio_pci_id_table[] = {
> >  
> >  MODULE_DEVICE_TABLE(pci, virtio_pci_id_table);
> >  
> > -/* A PCI device has it's own struct device and so does a virtio device so
> > - * we create a place for the virtio devices to show up in sysfs.  I think it
> > - * would make more sense for virtio to not insist on having it's own device. */
> > -static struct device *virtio_pci_root;
> > -
> >  /* Convert a generic virtio device to our structure */
> >  static struct virtio_pci_device *to_vp_device(struct virtio_device *vdev)
> >  {
> > @@ -629,7 +624,7 @@ static int __devinit virtio_pci_probe(struct pci_dev *pci_dev,
> >  	if (vp_dev == NULL)
> >  		return -ENOMEM;
> >  
> > -	vp_dev->vdev.dev.parent = virtio_pci_root;
> > +	vp_dev->vdev.dev.parent = &pci_dev->dev;
> >  	vp_dev->vdev.dev.release = virtio_pci_release_dev;
> >  	vp_dev->vdev.config = &virtio_pci_config_ops;
> >  	vp_dev->pci_dev = pci_dev;
> > @@ -717,17 +712,7 @@ static struct pci_driver virtio_pci_driver = {
> >  
> >  static int __init virtio_pci_init(void)
> >  {
> > -	int err;
> > -
> > -	virtio_pci_root = root_device_register("virtio-pci");
> > -	if (IS_ERR(virtio_pci_root))
> > -		return PTR_ERR(virtio_pci_root);
> > -
> > -	err = pci_register_driver(&virtio_pci_driver);
> > -	if (err)
> > -		root_device_unregister(virtio_pci_root);
> > -
> > -	return err;
> > +	return pci_register_driver(&virtio_pci_driver);
> >  }
> >  
> >  module_init(virtio_pci_init);
> > @@ -735,7 +720,6 @@ module_init(virtio_pci_init);
> >  static void __exit virtio_pci_exit(void)
> >  {
> >  	pci_unregister_driver(&virtio_pci_driver);
> > -	root_device_unregister(virtio_pci_root);
> >  }
> >  
> >  module_exit(virtio_pci_exit);
> 
> --
> 			Gleb.

Re: [PATCH] virtio: remove virtio-pci root device

[Rusty Russell]

On Tue, 11 Jan 2011 03:49:04 am Michael S. Tsirkin wrote:
> On Mon, Jan 10, 2011 at 07:10:46PM +0200, Gleb Natapov wrote:
> > On Fri, Jan 07, 2011 at 02:55:06AM -0600, Milton Miller wrote:
> > > We sometimes need to map between the virtio device and
> > > the given pci device. One such use is OS installer that
> > > gets the boot pci device from BIOS and needs to
> > > find the relevant block device. Since it can't,
> > > installation fails.
> > > 
> > > Instead of creating a top-level devices/virtio-pci
> > > directory, create each device under the corresponding
> > > pci device node.  Symlinks to all virtio-pci
> > > devices can be found under the pci driver link in
> > > bus/pci/drivers/virtio-pci/devices, and all virtio
> > > devices under drivers/bus/virtio/devices.
> > > 
> > > Signed-off-by: Milton Miller <miltonm@bga.com>
> > Acked-by: Gleb Natapov <gleb@redhat.com>
> 
> Rusty, any comments?
> We are going to tell the installer guys that this
> is the interface, would be nice to know that the
> patch is queued before we do.

Yep, I've applied it.  Sorry, took a week off to work on my linux.conf.au
talk...

Cheers,
Rusty.

Re: [PATCH] virtio: remove virtio-pci root device

[Michael S. Tsirkin]

On Fri, Jan 07, 2011 at 02:55:06AM -0600, Milton Miller wrote:
> We sometimes need to map between the virtio device and
> the given pci device. One such use is OS installer that
> gets the boot pci device from BIOS and needs to
> find the relevant block device. Since it can't,
> installation fails.
> 
> Instead of creating a top-level devices/virtio-pci
> directory, create each device under the corresponding
> pci device node.  Symlinks to all virtio-pci
> devices can be found under the pci driver link in
> bus/pci/drivers/virtio-pci/devices, and all virtio
> devices under drivers/bus/virtio/devices.
> 
> Signed-off-by: Milton Miller <miltonm@bga.com>

Tested-by: "Daniel P. Berrange" <berrange@redhat.com>

> ---
> 
> This is an alternative to the patch by Michael S. Tsirkin
> titled "virtio-pci: add softlinks between virtio and pci"
> https://patchwork.kernel.org/patch/454581/
> 
> It creates simpler code, uses less memory, and should
> be even easier use by the installer as it won't have to
> know a virtio symlink to follow (just follow none).
> 
> Compile tested only as I don't have kvm setup.
> 
> 
> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> index ef8d9d5..4fb5b2b 100644
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -96,11 +96,6 @@ static struct pci_device_id virtio_pci_id_table[] = {
>  
>  MODULE_DEVICE_TABLE(pci, virtio_pci_id_table);
>  
> -/* A PCI device has it's own struct device and so does a virtio device so
> - * we create a place for the virtio devices to show up in sysfs.  I think it
> - * would make more sense for virtio to not insist on having it's own device. */
> -static struct device *virtio_pci_root;
> -
>  /* Convert a generic virtio device to our structure */
>  static struct virtio_pci_device *to_vp_device(struct virtio_device *vdev)
>  {
> @@ -629,7 +624,7 @@ static int __devinit virtio_pci_probe(struct pci_dev *pci_dev,
>  	if (vp_dev == NULL)
>  		return -ENOMEM;
>  
> -	vp_dev->vdev.dev.parent = virtio_pci_root;
> +	vp_dev->vdev.dev.parent = &pci_dev->dev;
>  	vp_dev->vdev.dev.release = virtio_pci_release_dev;
>  	vp_dev->vdev.config = &virtio_pci_config_ops;
>  	vp_dev->pci_dev = pci_dev;
> @@ -717,17 +712,7 @@ static struct pci_driver virtio_pci_driver = {
>  
>  static int __init virtio_pci_init(void)
>  {
> -	int err;
> -
> -	virtio_pci_root = root_device_register("virtio-pci");
> -	if (IS_ERR(virtio_pci_root))
> -		return PTR_ERR(virtio_pci_root);
> -
> -	err = pci_register_driver(&virtio_pci_driver);
> -	if (err)
> -		root_device_unregister(virtio_pci_root);
> -
> -	return err;
> +	return pci_register_driver(&virtio_pci_driver);
>  }
>  
>  module_init(virtio_pci_init);
> @@ -735,7 +720,6 @@ module_init(virtio_pci_init);
>  static void __exit virtio_pci_exit(void)
>  {
>  	pci_unregister_driver(&virtio_pci_driver);
> -	root_device_unregister(virtio_pci_root);
>  }
>  
>  module_exit(virtio_pci_exit);