[PATCH v1] Documentation and two bug-fixes for Xen pciback.

[PATCH v1] Documentation and two bug-fixes for Xen pciback.

[Konrad Rzeszutek Wilk]

Hey,

This patchset is just a simple documentation and two bug-fixes
to the Xen pciback. I am still looking at implementing the proper
way of doing reset for the devices - based on David's feedback - but
that is going to take a big of time. In the meantime this patchset
is presented as today is the documentation day!


 drivers/xen/xen-pciback/pci_stub.c | 25 +++++++++++++++++++------
 drivers/xen/xen-pciback/xenbus.c   |  4 ++++
 2 files changed, 23 insertions(+), 6 deletions(-)

Konrad Rzeszutek Wilk (6):
      xen-pciback: Cleanup up pcistub_put_pci_dev
      xen-pciback: First reset, then free.
      xen-pciback: Document when we FLR an PCI device.
      xen/pciback: Document when the 'unbind' and 'bind' functions are called.
      xen/pciback: Document the entry points for 'pcistub_put_pci_dev'
      xen/pciback: Don't call xen_pcibk_config_init_dev when device de-assigned.

[PATCH v1 1/6] xen-pciback: Cleanup up pcistub_put_pci_dev

[Konrad Rzeszutek Wilk]

We are using 'psdev->dev','found_psdev->dev', and 'dev' at the
same time - and they all point to the same structure.

To keep it straight lets just use one - 'dev'.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
 drivers/xen/xen-pciback/pci_stub.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
index 62fcd48..5300a21 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -272,16 +272,16 @@ void pcistub_put_pci_dev(struct pci_dev *dev)
 	 * and want to inhibit the user from fiddling with 'reset'
 	 */
 	pci_reset_function(dev);
-	pci_restore_state(psdev->dev);
+	pci_restore_state(dev);
 
 	/* This disables the device. */
-	xen_pcibk_reset_device(found_psdev->dev);
+	xen_pcibk_reset_device(dev);
 
 	/* And cleanup up our emulated fields. */
-	xen_pcibk_config_free_dyn_fields(found_psdev->dev);
-	xen_pcibk_config_reset_dev(found_psdev->dev);
+	xen_pcibk_config_free_dyn_fields(dev);
+	xen_pcibk_config_reset_dev(dev);
 
-	xen_unregister_device_domain_owner(found_psdev->dev);
+	xen_unregister_device_domain_owner(dev);
 
 	spin_lock_irqsave(&found_psdev->lock, flags);
 	found_psdev->pdev = NULL;
-- 
1.8.5.3

[PATCH v1 1/6] xen-pciback: Cleanup up pcistub_put_pci_dev

[Konrad Rzeszutek Wilk]

We are using 'psdev->dev','found_psdev->dev', and 'dev' at the
same time - and they all point to the same structure.

To keep it straight lets just use one - 'dev'.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
 drivers/xen/xen-pciback/pci_stub.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
index 62fcd48..5300a21 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -272,16 +272,16 @@ void pcistub_put_pci_dev(struct pci_dev *dev)
 	 * and want to inhibit the user from fiddling with 'reset'
 	 */
 	pci_reset_function(dev);
-	pci_restore_state(psdev->dev);
+	pci_restore_state(dev);
 
 	/* This disables the device. */
-	xen_pcibk_reset_device(found_psdev->dev);
+	xen_pcibk_reset_device(dev);
 
 	/* And cleanup up our emulated fields. */
-	xen_pcibk_config_free_dyn_fields(found_psdev->dev);
-	xen_pcibk_config_reset_dev(found_psdev->dev);
+	xen_pcibk_config_free_dyn_fields(dev);
+	xen_pcibk_config_reset_dev(dev);
 
-	xen_unregister_device_domain_owner(found_psdev->dev);
+	xen_unregister_device_domain_owner(dev);
 
 	spin_lock_irqsave(&found_psdev->lock, flags);
 	found_psdev->pdev = NULL;
-- 
1.8.5.3

[PATCH v1 2/6] xen-pciback: First reset, then free.

[Konrad Rzeszutek Wilk]

We were doing the operations of freeing and reset in the wrong
order. Granted nothing broke because the reset functions just
set bar->which = 0.

But nonethless this was incorrect.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
 drivers/xen/xen-pciback/pci_stub.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
index 5300a21..36dd4f3 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -278,8 +278,8 @@ void pcistub_put_pci_dev(struct pci_dev *dev)
 	xen_pcibk_reset_device(dev);
 
 	/* And cleanup up our emulated fields. */
-	xen_pcibk_config_free_dyn_fields(dev);
 	xen_pcibk_config_reset_dev(dev);
+	xen_pcibk_config_free_dyn_fields(dev);
 
 	xen_unregister_device_domain_owner(dev);
 
-- 
1.8.5.3

[PATCH v1 2/6] xen-pciback: First reset, then free.

[Konrad Rzeszutek Wilk]

We were doing the operations of freeing and reset in the wrong
order. Granted nothing broke because the reset functions just
set bar->which = 0.

But nonethless this was incorrect.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
 drivers/xen/xen-pciback/pci_stub.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
index 5300a21..36dd4f3 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -278,8 +278,8 @@ void pcistub_put_pci_dev(struct pci_dev *dev)
 	xen_pcibk_reset_device(dev);
 
 	/* And cleanup up our emulated fields. */
-	xen_pcibk_config_free_dyn_fields(dev);
 	xen_pcibk_config_reset_dev(dev);
+	xen_pcibk_config_free_dyn_fields(dev);
 
 	xen_unregister_device_domain_owner(dev);
 
-- 
1.8.5.3

[PATCH v1 3/6] xen-pciback: Document when we FLR an PCI device.

[Konrad Rzeszutek Wilk]

When the toolstack wants us to drop or add an PCI device it
changes the XenBus state to Configuring - and as result of that
we find out which devices we should still be exporting out and
which ones not. For the ones we don't need anymore we need to
do an PCI reset so that it is ready for the next guest.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/xen/xen-pciback/pci_stub.c | 2 ++
 drivers/xen/xen-pciback/xenbus.c   | 4 ++++
 2 files changed, 6 insertions(+)

diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
index 36dd4f3..b84426a 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -551,6 +551,8 @@ static void pcistub_remove(struct pci_dev *dev)
 			pr_warn("****** shutdown driver domain before binding device\n");
 			pr_warn("****** to other drivers or domains\n");
 
+			/* N.B. This ends up calling pcistub_put_pci_dev which ends up
+			 * doing the FLR. */
 			xen_pcibk_release_pci_dev(found_psdev->pdev,
 						found_psdev->dev);
 		}
diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
index a9ed867..4a7e6e0 100644
--- a/drivers/xen/xen-pciback/xenbus.c
+++ b/drivers/xen/xen-pciback/xenbus.c
@@ -93,6 +93,8 @@ static void free_pdev(struct xen_pcibk_device *pdev)
 
 	xen_pcibk_disconnect(pdev);
 
+	/* N.B. This calls pcistub_put_pci_dev which does the FLR on all
+	 * of the PCIe devices. */
 	xen_pcibk_release_devices(pdev);
 
 	dev_set_drvdata(&pdev->xdev->dev, NULL);
@@ -286,6 +288,8 @@ static int xen_pcibk_remove_device(struct xen_pcibk_device *pdev,
 	dev_dbg(&dev->dev, "unregistering for %d\n", pdev->xdev->otherend_id);
 	xen_unregister_device_domain_owner(dev);
 
+	/* N.B. This ends up calling pcistub_put_pci_dev which ends up
+	 * doing the FLR. */
 	xen_pcibk_release_pci_dev(pdev, dev);
 
 out:
-- 
1.8.5.3

[PATCH v1 4/6] xen/pciback: Document when the 'unbind' and 'bind' functions are called.

[Konrad Rzeszutek Wilk]

And also mention that you cannot do any pci_reset_function,
pci_reset_slot, or such calls. This is because they take the same
lock as SysFS does - and we would end up with a dead-lock if
we call those functions.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/xen/xen-pciback/pci_stub.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
index b84426a..1539bec 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -493,6 +493,8 @@ static int pcistub_seize(struct pci_dev *dev)
 	return err;
 }
 
+/* Called when 'bind'. This means we must _NOT_ call pci_reset_function or
+ * other functions that take the sysfs lock. */
 static int pcistub_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
 	int err = 0;
@@ -520,6 +522,8 @@ out:
 	return err;
 }
 
+/* Called when 'unbind'. This means we must _NOT_ call pci_reset_function or
+ * other functions that take the sysfs lock. */
 static void pcistub_remove(struct pci_dev *dev)
 {
 	struct pcistub_device *psdev, *found_psdev = NULL;
-- 
1.8.5.3

[PATCH v1 4/6] xen/pciback: Document when the 'unbind' and 'bind' functions are called.

[Konrad Rzeszutek Wilk]

And also mention that you cannot do any pci_reset_function,
pci_reset_slot, or such calls. This is because they take the same
lock as SysFS does - and we would end up with a dead-lock if
we call those functions.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/xen/xen-pciback/pci_stub.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
index b84426a..1539bec 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -493,6 +493,8 @@ static int pcistub_seize(struct pci_dev *dev)
 	return err;
 }
 
+/* Called when 'bind'. This means we must _NOT_ call pci_reset_function or
+ * other functions that take the sysfs lock. */
 static int pcistub_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
 	int err = 0;
@@ -520,6 +522,8 @@ out:
 	return err;
 }
 
+/* Called when 'unbind'. This means we must _NOT_ call pci_reset_function or
+ * other functions that take the sysfs lock. */
 static void pcistub_remove(struct pci_dev *dev)
 {
 	struct pcistub_device *psdev, *found_psdev = NULL;
-- 
1.8.5.3

[PATCH v1 5/6] xen/pciback: Document the entry points for 'pcistub_put_pci_dev'

[Konrad Rzeszutek Wilk]

which are quite a few. It should be evident that dealing with that
many options is a bit complex.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/xen/xen-pciback/pci_stub.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
index 1539bec..d57a173 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -242,6 +242,15 @@ struct pci_dev *pcistub_get_pci_dev(struct xen_pcibk_device *pdev,
 	return found_dev;
 }
 
+/*
+ * Called when:
+ *  - XenBus state has been reconfigure (pci unplug). See xen_pcibk_remove_device
+ *  - XenBus state has been disconnected (guest shutdown). See xen_pcibk_xenbus_remove
+ *  - 'echo BDF > unbind' on pciback module with no guest attached. See pcistub_remove
+ *  - 'echo BDF > unbind' with a guest still using it. See pcistub_remove
+ *
+ *  As such we have to be careful.
+ */
 void pcistub_put_pci_dev(struct pci_dev *dev)
 {
 	struct pcistub_device *psdev, *found_psdev = NULL;
-- 
1.8.5.3

[PATCH v1 5/6] xen/pciback: Document the entry points for 'pcistub_put_pci_dev'

[Konrad Rzeszutek Wilk]

which are quite a few. It should be evident that dealing with that
many options is a bit complex.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/xen/xen-pciback/pci_stub.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
index 1539bec..d57a173 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -242,6 +242,15 @@ struct pci_dev *pcistub_get_pci_dev(struct xen_pcibk_device *pdev,
 	return found_dev;
 }
 
+/*
+ * Called when:
+ *  - XenBus state has been reconfigure (pci unplug). See xen_pcibk_remove_device
+ *  - XenBus state has been disconnected (guest shutdown). See xen_pcibk_xenbus_remove
+ *  - 'echo BDF > unbind' on pciback module with no guest attached. See pcistub_remove
+ *  - 'echo BDF > unbind' with a guest still using it. See pcistub_remove
+ *
+ *  As such we have to be careful.
+ */
 void pcistub_put_pci_dev(struct pci_dev *dev)
 {
 	struct pcistub_device *psdev, *found_psdev = NULL;
-- 
1.8.5.3

[PATCH v1 6/6] xen/pciback: Don't call xen_pcibk_config_init_dev when device de-assigned.

[Konrad Rzeszutek Wilk]

When the device is de-assigned from a guest (but still owned
by xen-pciback) we would needlessly free all of its dynamic
entries.

That we should not do - that is only to be done when the device has
been removed from xen-pciback. That is, when the reference count
has reached zero - and we end up calling pcistub_device_release
which does this.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/xen/xen-pciback/pci_stub.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
index d57a173..158d53df 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -288,8 +288,6 @@ void pcistub_put_pci_dev(struct pci_dev *dev)
 
 	/* And cleanup up our emulated fields. */
 	xen_pcibk_config_reset_dev(dev);
-	xen_pcibk_config_free_dyn_fields(dev);
-
 	xen_unregister_device_domain_owner(dev);
 
 	spin_lock_irqsave(&found_psdev->lock, flags);
-- 
1.8.5.3

[PATCH v1 6/6] xen/pciback: Don't call xen_pcibk_config_init_dev when device de-assigned.

[Konrad Rzeszutek Wilk]

When the device is de-assigned from a guest (but still owned
by xen-pciback) we would needlessly free all of its dynamic
entries.

That we should not do - that is only to be done when the device has
been removed from xen-pciback. That is, when the reference count
has reached zero - and we end up calling pcistub_device_release
which does this.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/xen/xen-pciback/pci_stub.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
index d57a173..158d53df 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -288,8 +288,6 @@ void pcistub_put_pci_dev(struct pci_dev *dev)
 
 	/* And cleanup up our emulated fields. */
 	xen_pcibk_config_reset_dev(dev);
-	xen_pcibk_config_free_dyn_fields(dev);
-
 	xen_unregister_device_domain_owner(dev);
 
 	spin_lock_irqsave(&found_psdev->lock, flags);
-- 
1.8.5.3

Re: [PATCH v1 6/6] xen/pciback: Don't call xen_pcibk_config_init_dev when device de-assigned.

[Jan Beulich]

>>> On 30.04.14 at 15:54, <konrad.wilk@oracle.com> wrote:
> When the device is de-assigned from a guest (but still owned
> by xen-pciback) we would needlessly free all of its dynamic
> entries.

I wonder whether that isn't intentional - dynamic fields are only
being used for what comes through the "quirks" attribute.

> --- a/drivers/xen/xen-pciback/pci_stub.c
> +++ b/drivers/xen/xen-pciback/pci_stub.c
> @@ -288,8 +288,6 @@ void pcistub_put_pci_dev(struct pci_dev *dev)
>  
>  	/* And cleanup up our emulated fields. */
>  	xen_pcibk_config_reset_dev(dev);
> -	xen_pcibk_config_free_dyn_fields(dev);
> -

Irrespective of the above, patch title and contents are out of sync.

Jan

Re: [PATCH v1 6/6] xen/pciback: Don't call xen_pcibk_config_init_dev when device de-assigned.

[Jan Beulich]

>>> On 30.04.14 at 15:54, <konrad.wilk@oracle.com> wrote:
> When the device is de-assigned from a guest (but still owned
> by xen-pciback) we would needlessly free all of its dynamic
> entries.

I wonder whether that isn't intentional - dynamic fields are only
being used for what comes through the "quirks" attribute.

> --- a/drivers/xen/xen-pciback/pci_stub.c
> +++ b/drivers/xen/xen-pciback/pci_stub.c
> @@ -288,8 +288,6 @@ void pcistub_put_pci_dev(struct pci_dev *dev)
>  
>  	/* And cleanup up our emulated fields. */
>  	xen_pcibk_config_reset_dev(dev);
> -	xen_pcibk_config_free_dyn_fields(dev);
> -

Irrespective of the above, patch title and contents are out of sync.

Jan

Re: [PATCH v1] Documentation and two bug-fixes for Xen pciback.

[David Vrabel]

On 30/04/14 14:54, Konrad Rzeszutek Wilk wrote:
> Hey,
> 
> This patchset is just a simple documentation and two bug-fixes
> to the Xen pciback. I am still looking at implementing the proper
> way of doing reset for the devices - based on David's feedback - but
> that is going to take a big of time. In the meantime this patchset
> is presented as today is the documentation day!
> 
> 
>  drivers/xen/xen-pciback/pci_stub.c | 25 +++++++++++++++++++------
>  drivers/xen/xen-pciback/xenbus.c   |  4 ++++
>  2 files changed, 23 insertions(+), 6 deletions(-)
> 
> Konrad Rzeszutek Wilk (6):
>       xen-pciback: Cleanup up pcistub_put_pci_dev
>       xen-pciback: First reset, then free.
>       xen-pciback: Document when we FLR an PCI device.
>       xen/pciback: Document when the 'unbind' and 'bind' functions are called.
>       xen/pciback: Document the entry points for 'pcistub_put_pci_dev'

These:

Reviewed-by: David Vrabel <david.vrabel@citrix.com>

>       xen/pciback: Don't call xen_pcibk_config_init_dev when device
de-assigned.

Could you just check that nothing relied on the dyn_fields() being
deleted here.

David

Re: [PATCH v1] Documentation and two bug-fixes for Xen pciback.

[David Vrabel]

On 30/04/14 14:54, Konrad Rzeszutek Wilk wrote:
> Hey,
> 
> This patchset is just a simple documentation and two bug-fixes
> to the Xen pciback. I am still looking at implementing the proper
> way of doing reset for the devices - based on David's feedback - but
> that is going to take a big of time. In the meantime this patchset
> is presented as today is the documentation day!
> 
> 
>  drivers/xen/xen-pciback/pci_stub.c | 25 +++++++++++++++++++------
>  drivers/xen/xen-pciback/xenbus.c   |  4 ++++
>  2 files changed, 23 insertions(+), 6 deletions(-)
> 
> Konrad Rzeszutek Wilk (6):
>       xen-pciback: Cleanup up pcistub_put_pci_dev
>       xen-pciback: First reset, then free.
>       xen-pciback: Document when we FLR an PCI device.
>       xen/pciback: Document when the 'unbind' and 'bind' functions are called.
>       xen/pciback: Document the entry points for 'pcistub_put_pci_dev'

These:

Reviewed-by: David Vrabel <david.vrabel@citrix.com>

>       xen/pciback: Don't call xen_pcibk_config_init_dev when device
de-assigned.

Could you just check that nothing relied on the dyn_fields() being
deleted here.

David

[PATCH v1] Documentation and two bug-fixes for Xen pciback.

[Konrad Rzeszutek Wilk]

Hey,

This patchset is just a simple documentation and two bug-fixes
to the Xen pciback. I am still looking at implementing the proper
way of doing reset for the devices - based on David's feedback - but
that is going to take a big of time. In the meantime this patchset
is presented as today is the documentation day!


 drivers/xen/xen-pciback/pci_stub.c | 25 +++++++++++++++++++------
 drivers/xen/xen-pciback/xenbus.c   |  4 ++++
 2 files changed, 23 insertions(+), 6 deletions(-)

Konrad Rzeszutek Wilk (6):
      xen-pciback: Cleanup up pcistub_put_pci_dev
      xen-pciback: First reset, then free.
      xen-pciback: Document when we FLR an PCI device.
      xen/pciback: Document when the 'unbind' and 'bind' functions are called.
      xen/pciback: Document the entry points for 'pcistub_put_pci_dev'
      xen/pciback: Don't call xen_pcibk_config_init_dev when device de-assigned.