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.
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 <[email protected]>
Reviewed-by: Jan Beulich <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Jan Beulich <[email protected]>
---
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
>>> On 30.04.14 at 15:54, <[email protected]> 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
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 <[email protected]>
> 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