Hey,
The last time I posted these patches: https://lkml.org/lkml/2014/7/8/533
was so long ago that I don't even remember most comments. The only
one that stuck in mind was David's recommendation to add a new PCI API
call and use that. See patch #6 and #7.
The original posting (v3) had an extra patch that would do slot and
bus reset using do_flr SysFS attribute. I will revisit that once I am
done with this patchset.
I also seem to have had in my a bunch of 'SoB' from David - which
makes no sense - unless I pulled from his tree. Anyhow wherewere
I saw them I removed them.
Please take a look and comment. If I missed some comment from months
ago hopefully the new version has clarified them.
drivers/pci/pci.c | 5 +--
drivers/xen/xen-pciback/passthrough.c | 14 ++++++--
drivers/xen/xen-pciback/pci_stub.c | 60 ++++++++++++++++++++++-------------
drivers/xen/xen-pciback/pciback.h | 7 ++--
drivers/xen/xen-pciback/vpci.c | 14 ++++++--
drivers/xen/xen-pciback/xenbus.c | 4 +--
include/linux/device.h | 5 +++
include/linux/pci.h | 2 ++
8 files changed, 76 insertions(+), 35 deletions(-)
Konrad Rzeszutek Wilk (7):
xen/pciback: Don't deadlock when unbinding.
driver core: Provide an wrapper around the mutex to do lockdep warnings
xen/pciback: Include the domain id if removing the device whilst still in use
xen/pciback: Print out the domain owning the device.
xen/pciback: Remove tons of dereferences
PCI: Expose pci_load_saved_state for public consumption.
xen/pciback: Restore configuration space when detaching from a guest.
We had been printing it only if the device was built with
debug enabled. But this information is useful in the field
to troubleshoot.
Signed-off-by: Konrad Rzeszutek Wilk <[email protected]>
Reviewed-by: David Vrabel <[email protected]>
---
drivers/xen/xen-pciback/xenbus.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
index eeee499..fe17c80 100644
--- a/drivers/xen/xen-pciback/xenbus.c
+++ b/drivers/xen/xen-pciback/xenbus.c
@@ -247,7 +247,7 @@ static int xen_pcibk_export_device(struct xen_pcibk_device *pdev,
if (err)
goto out;
- dev_dbg(&dev->dev, "registering for %d\n", pdev->xdev->otherend_id);
+ dev_info(&dev->dev, "registering for %d\n", pdev->xdev->otherend_id);
if (xen_register_device_domain_owner(dev,
pdev->xdev->otherend_id) != 0) {
dev_err(&dev->dev, "Stealing ownership from dom%d.\n",
--
1.9.3
Instead of open-coding it in drivers that want to double check
that their functions are indeed holding the device lock.
Signed-off-by: Konrad Rzeszutek Wilk <[email protected]>
Suggested-by: David Vrabel <[email protected]>
Acked-by: Greg Kroah-Hartman <[email protected]>
---
drivers/xen/xen-pciback/pci_stub.c | 2 +-
include/linux/device.h | 5 +++++
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
index 9cbe1a3..8b77089 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -278,7 +278,7 @@ void pcistub_put_pci_dev(struct pci_dev *dev)
/* Cleanup our device
* (so it's ready for the next domain)
*/
- lockdep_assert_held(&dev->dev.mutex);
+ device_lock_assert(&dev->dev);
__pci_reset_function_locked(dev);
pci_restore_state(dev);
diff --git a/include/linux/device.h b/include/linux/device.h
index ce1f2160..41d6a75 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -911,6 +911,11 @@ static inline void device_unlock(struct device *dev)
mutex_unlock(&dev->mutex);
}
+static inline void device_lock_assert(struct device *dev)
+{
+ lockdep_assert_held(&dev->mutex);
+}
+
void driver_init(void);
/*
--
1.9.3
The commit "xen/pciback: Don't deadlock when unbinding." was using
the version of pci_reset_function which would lock the device lock.
That is no good as we can dead-lock. As such we swapped to using
the lock-less version and requiring that the callers
of 'pcistub_put_pci_dev' take the device lock. And as such
this bug got exposed.
Using the lock-less version is OK, except that we tried to
use 'pci_restore_state' after the lock-less version of
__pci_reset_function_locked - which won't work as 'state_saved'
is set to false. Said 'state_saved' is a toggle boolean that
is to be used by the sequence of a) pci_save_state/pci_restore_state
or b) pci_load_and_free_saved_state/pci_restore_state. We don't
want to use a) as the guest might have messed up the PCI
configuration space and we want it to revert to the state
when the PCI device was binded to us. Therefore we pick
b) to restore the configuration space.
We restore from our 'golden' version of PCI configuration space, when an:
- Device is unbinded from pciback
- Device is detached from a guest.
Reported-by: Sander Eikelenboom <[email protected]>
Signed-off-by: Konrad Rzeszutek Wilk <[email protected]>
---
drivers/xen/xen-pciback/pci_stub.c | 20 ++++++++++++++++----
1 file changed, 16 insertions(+), 4 deletions(-)
diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
index 843a2ba..eb8b58e 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -105,7 +105,7 @@ static void pcistub_device_release(struct kref *kref)
*/
__pci_reset_function_locked(dev);
if (pci_load_and_free_saved_state(dev, &dev_data->pci_saved_state))
- dev_dbg(&dev->dev, "Could not reload PCI state\n");
+ dev_info(&dev->dev, "Could not reload PCI state\n");
else
pci_restore_state(dev);
@@ -257,6 +257,8 @@ void pcistub_put_pci_dev(struct pci_dev *dev)
{
struct pcistub_device *psdev, *found_psdev = NULL;
unsigned long flags;
+ struct xen_pcibk_dev_data *dev_data;
+ int ret;
spin_lock_irqsave(&pcistub_devices_lock, flags);
@@ -279,9 +281,19 @@ void pcistub_put_pci_dev(struct pci_dev *dev)
* (so it's ready for the next domain)
*/
device_lock_assert(&dev->dev);
- __pci_reset_function_locked(dev);
- pci_restore_state(dev);
-
+ dev_data = pci_get_drvdata(dev);
+ ret = pci_load_saved_state(dev, dev_data->pci_saved_state);
+ if (ret < 0)
+ dev_warn(&dev->dev, "Could not reload PCI state\n");
+ else {
+ __pci_reset_function_locked(dev);
+ /*
+ * The usual sequence is pci_save_state & pci_restore_state
+ * but the guest might have messed the configuration space up.
+ * Use the initial version (when device was bound to us).
+ */
+ pci_restore_state(dev);
+ }
/* This disables the device. */
xen_pcibk_reset_device(dev);
--
1.9.3
As commit 0a9fd0152929db372ff61b0d6c280fdd34ae8bdb
'xen/pciback: Document the entry points for 'pcistub_put_pci_dev''
explained there are four entry points in this function.
Two of them are when the user fiddles in the SysFS to
unbind a device which might be in use by a guest or not.
Both 'unbind' states will cause a deadlock as the the PCI lock has
already been taken, which then pci_device_reset tries to take.
We can simplify this by requiring that all callers of
pcistub_put_pci_dev MUST hold the device lock. And then
we can just call the lockless version of pci_device_reset.
To make it even simpler we will modify xen_pcibk_release_pci_dev
to quality whether it should take a lock or not - as it ends
up calling xen_pcibk_release_pci_dev and needs to hold the lock.
Reviewed-by: Boris Ostrovsky <[email protected]>
Signed-off-by: Konrad Rzeszutek Wilk <[email protected]>
---
drivers/xen/xen-pciback/passthrough.c | 14 +++++++++++---
drivers/xen/xen-pciback/pci_stub.c | 12 ++++++------
drivers/xen/xen-pciback/pciback.h | 7 ++++---
drivers/xen/xen-pciback/vpci.c | 14 +++++++++++---
drivers/xen/xen-pciback/xenbus.c | 2 +-
5 files changed, 33 insertions(+), 16 deletions(-)
diff --git a/drivers/xen/xen-pciback/passthrough.c b/drivers/xen/xen-pciback/passthrough.c
index 828dddc..f16a30e 100644
--- a/drivers/xen/xen-pciback/passthrough.c
+++ b/drivers/xen/xen-pciback/passthrough.c
@@ -69,7 +69,7 @@ static int __xen_pcibk_add_pci_dev(struct xen_pcibk_device *pdev,
}
static void __xen_pcibk_release_pci_dev(struct xen_pcibk_device *pdev,
- struct pci_dev *dev)
+ struct pci_dev *dev, bool lock)
{
struct passthrough_dev_data *dev_data = pdev->pci_dev_data;
struct pci_dev_entry *dev_entry, *t;
@@ -87,8 +87,13 @@ static void __xen_pcibk_release_pci_dev(struct xen_pcibk_device *pdev,
mutex_unlock(&dev_data->lock);
- if (found_dev)
+ if (found_dev) {
+ if (lock)
+ device_lock(&found_dev->dev);
pcistub_put_pci_dev(found_dev);
+ if (lock)
+ device_unlock(&found_dev->dev);
+ }
}
static int __xen_pcibk_init_devices(struct xen_pcibk_device *pdev)
@@ -156,8 +161,11 @@ static void __xen_pcibk_release_devices(struct xen_pcibk_device *pdev)
struct pci_dev_entry *dev_entry, *t;
list_for_each_entry_safe(dev_entry, t, &dev_data->dev_list, list) {
+ struct pci_dev *dev = dev_entry->dev;
list_del(&dev_entry->list);
- pcistub_put_pci_dev(dev_entry->dev);
+ device_lock(&dev->dev);
+ pcistub_put_pci_dev(dev);
+ device_unlock(&dev->dev);
kfree(dev_entry);
}
diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
index 017069a..9cbe1a3 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -250,6 +250,8 @@ struct pci_dev *pcistub_get_pci_dev(struct xen_pcibk_device *pdev,
* - 'echo BDF > unbind' with a guest still using it. See pcistub_remove
*
* As such we have to be careful.
+ *
+ * To make this easier, the caller has to hold the device lock.
*/
void pcistub_put_pci_dev(struct pci_dev *dev)
{
@@ -276,11 +278,8 @@ void pcistub_put_pci_dev(struct pci_dev *dev)
/* Cleanup our device
* (so it's ready for the next domain)
*/
-
- /* This is OK - we are running from workqueue context
- * and want to inhibit the user from fiddling with 'reset'
- */
- pci_reset_function(dev);
+ lockdep_assert_held(&dev->dev.mutex);
+ __pci_reset_function_locked(dev);
pci_restore_state(dev);
/* This disables the device. */
@@ -567,7 +566,8 @@ static void pcistub_remove(struct pci_dev *dev)
/* 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);
+ found_psdev->dev,
+ false /* caller holds the lock. */);
}
spin_lock_irqsave(&pcistub_devices_lock, flags);
diff --git a/drivers/xen/xen-pciback/pciback.h b/drivers/xen/xen-pciback/pciback.h
index f72af87..58e38d5 100644
--- a/drivers/xen/xen-pciback/pciback.h
+++ b/drivers/xen/xen-pciback/pciback.h
@@ -99,7 +99,8 @@ struct xen_pcibk_backend {
unsigned int *domain, unsigned int *bus,
unsigned int *devfn);
int (*publish)(struct xen_pcibk_device *pdev, publish_pci_root_cb cb);
- void (*release)(struct xen_pcibk_device *pdev, struct pci_dev *dev);
+ void (*release)(struct xen_pcibk_device *pdev, struct pci_dev *dev,
+ bool lock);
int (*add)(struct xen_pcibk_device *pdev, struct pci_dev *dev,
int devid, publish_pci_dev_cb publish_cb);
struct pci_dev *(*get)(struct xen_pcibk_device *pdev,
@@ -122,10 +123,10 @@ static inline int xen_pcibk_add_pci_dev(struct xen_pcibk_device *pdev,
}
static inline void xen_pcibk_release_pci_dev(struct xen_pcibk_device *pdev,
- struct pci_dev *dev)
+ struct pci_dev *dev, bool lock)
{
if (xen_pcibk_backend && xen_pcibk_backend->release)
- return xen_pcibk_backend->release(pdev, dev);
+ return xen_pcibk_backend->release(pdev, dev, lock);
}
static inline struct pci_dev *
diff --git a/drivers/xen/xen-pciback/vpci.c b/drivers/xen/xen-pciback/vpci.c
index 51afff9..c99f8bb 100644
--- a/drivers/xen/xen-pciback/vpci.c
+++ b/drivers/xen/xen-pciback/vpci.c
@@ -145,7 +145,7 @@ out:
}
static void __xen_pcibk_release_pci_dev(struct xen_pcibk_device *pdev,
- struct pci_dev *dev)
+ struct pci_dev *dev, bool lock)
{
int slot;
struct vpci_dev_data *vpci_dev = pdev->pci_dev_data;
@@ -169,8 +169,13 @@ static void __xen_pcibk_release_pci_dev(struct xen_pcibk_device *pdev,
out:
mutex_unlock(&vpci_dev->lock);
- if (found_dev)
+ if (found_dev) {
+ if (lock)
+ device_lock(&found_dev->dev);
pcistub_put_pci_dev(found_dev);
+ if (lock)
+ device_unlock(&found_dev->dev);
+ }
}
static int __xen_pcibk_init_devices(struct xen_pcibk_device *pdev)
@@ -208,8 +213,11 @@ static void __xen_pcibk_release_devices(struct xen_pcibk_device *pdev)
struct pci_dev_entry *e, *tmp;
list_for_each_entry_safe(e, tmp, &vpci_dev->dev_list[slot],
list) {
+ struct pci_dev *dev = e->dev;
list_del(&e->list);
- pcistub_put_pci_dev(e->dev);
+ device_lock(&dev->dev);
+ pcistub_put_pci_dev(dev);
+ device_unlock(&dev->dev);
kfree(e);
}
}
diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
index ad8d30c..eeee499 100644
--- a/drivers/xen/xen-pciback/xenbus.c
+++ b/drivers/xen/xen-pciback/xenbus.c
@@ -291,7 +291,7 @@ static int xen_pcibk_remove_device(struct xen_pcibk_device *pdev,
/* N.B. This ends up calling pcistub_put_pci_dev which ends up
* doing the FLR. */
- xen_pcibk_release_pci_dev(pdev, dev);
+ xen_pcibk_release_pci_dev(pdev, dev, true /* use the lock. */);
out:
return err;
--
1.9.3
Cleanup the function a bit - also include the id of the
domain that is using the device.
Signed-off-by: Konrad Rzeszutek Wilk <[email protected]>
Reviewed-by: David Vrabel <[email protected]>
---
drivers/xen/xen-pciback/pci_stub.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
index 8b77089..e5ff09d 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -553,12 +553,14 @@ static void pcistub_remove(struct pci_dev *dev)
spin_unlock_irqrestore(&pcistub_devices_lock, flags);
if (found_psdev) {
- dev_dbg(&dev->dev, "found device to remove - in use? %p\n",
- found_psdev->pdev);
+ dev_dbg(&dev->dev, "found device to remove %s\n",
+ found_psdev->pdev ? "- in-use" : "");
if (found_psdev->pdev) {
- pr_warn("****** removing device %s while still in-use! ******\n",
- pci_name(found_psdev->dev));
+ int domid = xen_find_device_domain_owner(dev);
+
+ pr_warn("****** removing device %s while still in-use by domain %d! ******\n",
+ pci_name(found_psdev->dev), domid);
pr_warn("****** driver domain may still access this device's i/o resources!\n");
pr_warn("****** shutdown driver domain before binding device\n");
pr_warn("****** to other drivers or domains\n");
--
1.9.3
A little cleanup. No functional difference.
Reviewed-by: Boris Ostrovsky <[email protected]>
Signed-off-by: Konrad Rzeszutek Wilk <[email protected]>
---
drivers/xen/xen-pciback/pci_stub.c | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
index e5ff09d..843a2ba 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -631,10 +631,12 @@ static pci_ers_result_t common_process(struct pcistub_device *psdev,
{
pci_ers_result_t res = result;
struct xen_pcie_aer_op *aer_op;
+ struct xen_pcibk_device *pdev = psdev->pdev;
+ struct xen_pci_sharedinfo *sh_info = pdev->sh_info;
int ret;
/*with PV AER drivers*/
- aer_op = &(psdev->pdev->sh_info->aer_op);
+ aer_op = &(sh_info->aer_op);
aer_op->cmd = aer_cmd ;
/*useful for error_detected callback*/
aer_op->err = state;
@@ -655,36 +657,36 @@ static pci_ers_result_t common_process(struct pcistub_device *psdev,
* this flag to judge whether we need to check pci-front give aer
* service ack signal
*/
- set_bit(_PCIB_op_pending, (unsigned long *)&psdev->pdev->flags);
+ set_bit(_PCIB_op_pending, (unsigned long *)&pdev->flags);
/*It is possible that a pcifront conf_read_write ops request invokes
* the callback which cause the spurious execution of wake_up.
* Yet it is harmless and better than a spinlock here
*/
set_bit(_XEN_PCIB_active,
- (unsigned long *)&psdev->pdev->sh_info->flags);
+ (unsigned long *)&sh_info->flags);
wmb();
- notify_remote_via_irq(psdev->pdev->evtchn_irq);
+ notify_remote_via_irq(pdev->evtchn_irq);
ret = wait_event_timeout(xen_pcibk_aer_wait_queue,
!(test_bit(_XEN_PCIB_active, (unsigned long *)
- &psdev->pdev->sh_info->flags)), 300*HZ);
+ &sh_info->flags)), 300*HZ);
if (!ret) {
if (test_bit(_XEN_PCIB_active,
- (unsigned long *)&psdev->pdev->sh_info->flags)) {
+ (unsigned long *)&sh_info->flags)) {
dev_err(&psdev->dev->dev,
"pcifront aer process not responding!\n");
clear_bit(_XEN_PCIB_active,
- (unsigned long *)&psdev->pdev->sh_info->flags);
+ (unsigned long *)&sh_info->flags);
aer_op->err = PCI_ERS_RESULT_NONE;
return res;
}
}
- clear_bit(_PCIB_op_pending, (unsigned long *)&psdev->pdev->flags);
+ clear_bit(_PCIB_op_pending, (unsigned long *)&pdev->flags);
if (test_bit(_XEN_PCIF_active,
- (unsigned long *)&psdev->pdev->sh_info->flags)) {
+ (unsigned long *)&sh_info->flags)) {
dev_dbg(&psdev->dev->dev,
"schedule pci_conf service in " DRV_NAME "\n");
xen_pcibk_test_and_schedule_op(psdev->pdev);
--
1.9.3
We have the pci_load_and_free_saved_state, and pci_store_saved_state
but are missing the functionality to just load the state
multiple times in the PCI device without having to free/save
the state.
This patch makes it possible to use this function.
CC: Bjorn Helgaas <[email protected]>
Signed-off-by: Konrad Rzeszutek Wilk <[email protected]>
---
drivers/pci/pci.c | 5 +++--
include/linux/pci.h | 2 ++
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 625a4ac..f00a9d6 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1142,8 +1142,8 @@ EXPORT_SYMBOL_GPL(pci_store_saved_state);
* @dev: PCI device that we're dealing with
* @state: Saved state returned from pci_store_saved_state()
*/
-static int pci_load_saved_state(struct pci_dev *dev,
- struct pci_saved_state *state)
+int pci_load_saved_state(struct pci_dev *dev,
+ struct pci_saved_state *state)
{
struct pci_cap_saved_data *cap;
@@ -1171,6 +1171,7 @@ static int pci_load_saved_state(struct pci_dev *dev,
dev->state_saved = true;
return 0;
}
+EXPORT_SYMBOL_GPL(pci_load_saved_state);
/**
* pci_load_and_free_saved_state - Reload the save state pointed to by state,
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 5be8db4..08088cb1 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1003,6 +1003,8 @@ void __iomem __must_check *pci_platform_rom(struct pci_dev *pdev, size_t *size);
int pci_save_state(struct pci_dev *dev);
void pci_restore_state(struct pci_dev *dev);
struct pci_saved_state *pci_store_saved_state(struct pci_dev *dev);
+int pci_load_saved_state(struct pci_dev *dev,
+ struct pci_saved_state *state);
int pci_load_and_free_saved_state(struct pci_dev *dev,
struct pci_saved_state **state);
struct pci_cap_saved_state *pci_find_saved_cap(struct pci_dev *dev, char cap);
--
1.9.3
On 21/11/14 22:17, Konrad Rzeszutek Wilk wrote:
> The commit "xen/pciback: Don't deadlock when unbinding." was using
> the version of pci_reset_function which would lock the device lock.
> That is no good as we can dead-lock. As such we swapped to using
> the lock-less version and requiring that the callers
> of 'pcistub_put_pci_dev' take the device lock. And as such
> this bug got exposed.
>
> Using the lock-less version is OK, except that we tried to
> use 'pci_restore_state' after the lock-less version of
> __pci_reset_function_locked - which won't work as 'state_saved'
> is set to false. Said 'state_saved' is a toggle boolean that
> is to be used by the sequence of a) pci_save_state/pci_restore_state
> or b) pci_load_and_free_saved_state/pci_restore_state. We don't
> want to use a) as the guest might have messed up the PCI
> configuration space and we want it to revert to the state
> when the PCI device was binded to us. Therefore we pick
> b) to restore the configuration space.
>
> We restore from our 'golden' version of PCI configuration space, when an:
> - Device is unbinded from pciback
> - Device is detached from a guest.
>
> Reported-by: Sander Eikelenboom <[email protected]>
> Signed-off-by: Konrad Rzeszutek Wilk <[email protected]>
> ---
> drivers/xen/xen-pciback/pci_stub.c | 20 ++++++++++++++++----
> 1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
> index 843a2ba..eb8b58e 100644
> --- a/drivers/xen/xen-pciback/pci_stub.c
> +++ b/drivers/xen/xen-pciback/pci_stub.c
> @@ -105,7 +105,7 @@ static void pcistub_device_release(struct kref *kref)
> */
> __pci_reset_function_locked(dev);
> if (pci_load_and_free_saved_state(dev, &dev_data->pci_saved_state))
> - dev_dbg(&dev->dev, "Could not reload PCI state\n");
> + dev_info(&dev->dev, "Could not reload PCI state\n");
Why dev_info when...
> @@ -279,9 +281,19 @@ void pcistub_put_pci_dev(struct pci_dev *dev)
> * (so it's ready for the next domain)
> */
> device_lock_assert(&dev->dev);
> - __pci_reset_function_locked(dev);
> - pci_restore_state(dev);
> -
> + dev_data = pci_get_drvdata(dev);
> + ret = pci_load_saved_state(dev, dev_data->pci_saved_state);
> + if (ret < 0)
> + dev_warn(&dev->dev, "Could not reload PCI state\n");
... this one is dev_warn?
> + else {
> + __pci_reset_function_locked(dev);
I think the reset should always be attempted regardless of whether the
correct state was loaded or not.
David
On Mon, Dec 01, 2014 at 02:14:56PM +0000, David Vrabel wrote:
> On 21/11/14 22:17, Konrad Rzeszutek Wilk wrote:
> > The commit "xen/pciback: Don't deadlock when unbinding." was using
> > the version of pci_reset_function which would lock the device lock.
> > That is no good as we can dead-lock. As such we swapped to using
> > the lock-less version and requiring that the callers
> > of 'pcistub_put_pci_dev' take the device lock. And as such
> > this bug got exposed.
> >
> > Using the lock-less version is OK, except that we tried to
> > use 'pci_restore_state' after the lock-less version of
> > __pci_reset_function_locked - which won't work as 'state_saved'
> > is set to false. Said 'state_saved' is a toggle boolean that
> > is to be used by the sequence of a) pci_save_state/pci_restore_state
> > or b) pci_load_and_free_saved_state/pci_restore_state. We don't
> > want to use a) as the guest might have messed up the PCI
> > configuration space and we want it to revert to the state
> > when the PCI device was binded to us. Therefore we pick
> > b) to restore the configuration space.
> >
> > We restore from our 'golden' version of PCI configuration space, when an:
> > - Device is unbinded from pciback
> > - Device is detached from a guest.
> >
> > Reported-by: Sander Eikelenboom <[email protected]>
> > Signed-off-by: Konrad Rzeszutek Wilk <[email protected]>
> > ---
> > drivers/xen/xen-pciback/pci_stub.c | 20 ++++++++++++++++----
> > 1 file changed, 16 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
> > index 843a2ba..eb8b58e 100644
> > --- a/drivers/xen/xen-pciback/pci_stub.c
> > +++ b/drivers/xen/xen-pciback/pci_stub.c
> > @@ -105,7 +105,7 @@ static void pcistub_device_release(struct kref *kref)
> > */
> > __pci_reset_function_locked(dev);
> > if (pci_load_and_free_saved_state(dev, &dev_data->pci_saved_state))
> > - dev_dbg(&dev->dev, "Could not reload PCI state\n");
> > + dev_info(&dev->dev, "Could not reload PCI state\n");
>
> Why dev_info when...
>
> > @@ -279,9 +281,19 @@ void pcistub_put_pci_dev(struct pci_dev *dev)
> > * (so it's ready for the next domain)
> > */
> > device_lock_assert(&dev->dev);
> > - __pci_reset_function_locked(dev);
> > - pci_restore_state(dev);
> > -
> > + dev_data = pci_get_drvdata(dev);
> > + ret = pci_load_saved_state(dev, dev_data->pci_saved_state);
> > + if (ret < 0)
> > + dev_warn(&dev->dev, "Could not reload PCI state\n");
>
> ... this one is dev_warn?
Should be the same, dev_info I think?
>
> > + else {
> > + __pci_reset_function_locked(dev);
>
> I think the reset should always be attempted regardless of whether the
> correct state was loaded or not.
/me nods. Will redo it as such.
>
> David
>>> On 21.11.14 at 23:17, <[email protected]> wrote:
> Konrad Rzeszutek Wilk (7):
> xen/pciback: Don't deadlock when unbinding.
> driver core: Provide an wrapper around the mutex to do lockdep warnings
> xen/pciback: Include the domain id if removing the device whilst still in use
> xen/pciback: Print out the domain owning the device.
> xen/pciback: Remove tons of dereferences
> PCI: Expose pci_load_saved_state for public consumption.
> xen/pciback: Restore configuration space when detaching from a guest.
So my "xen-pciback: drop SR-IOV VFs when PF driver unloads" isn't
among them, nor is there any alternative. What's the status of that
patch (or the problem that prompted its creation)?
Jan
On Tue, Dec 02, 2014 at 10:10:11AM +0000, Jan Beulich wrote:
> >>> On 21.11.14 at 23:17, <[email protected]> wrote:
> > Konrad Rzeszutek Wilk (7):
> > xen/pciback: Don't deadlock when unbinding.
> > driver core: Provide an wrapper around the mutex to do lockdep warnings
> > xen/pciback: Include the domain id if removing the device whilst still in use
> > xen/pciback: Print out the domain owning the device.
> > xen/pciback: Remove tons of dereferences
> > PCI: Expose pci_load_saved_state for public consumption.
> > xen/pciback: Restore configuration space when detaching from a guest.
>
> So my "xen-pciback: drop SR-IOV VFs when PF driver unloads" isn't
> among them, nor is there any alternative. What's the status of that
> patch (or the problem that prompted its creation)?
Oh, I've it in my queue. Um, here is what I did to it - I hadn't
yet tested it - hence the reason it is not on that list.
>From 82ad7b4cc73f2f9a9cfd6805cff996fd5009a31f Mon Sep 17 00:00:00 2001
From: Jan Beulich <[email protected]>
Date: Thu, 6 Nov 2014 15:05:51 +0000
Subject: [PATCH 1/2] xen-pciback: drop SR-IOV VFs when PF driver unloads
When a PF driver unloads, it may find it necessary to leave the VFs
around simply because of pciback having marked them as assigned to a
guest. Utilize a suitable notification to let go of the VFs, thus
allowing the PF to go back into the state it was before its driver
loaded (which in particular allows the driver to be loaded again with
it being able to create the VFs anew, but which also allows to then
pass through the PF instead of the VFs).
Don't do this however for any VFs currently in active use by a guest.
Signed-off-by: Jan Beulich <[email protected]>
[v2: Removed the switch statement, moved it about]
[v3: Redid it a bit differently]
Signed-off-by: Konrad Rzeszutek Wilk <[email protected]>
---
drivers/xen/xen-pciback/pci_stub.c | 54 ++++++++++++++++++++++++++++++++++++++
1 file changed, 54 insertions(+)
diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
index eb8b58e..ff27efa 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -1518,6 +1518,53 @@ parse_error:
fs_initcall(pcistub_init);
#endif
+#ifdef CONFIG_PCI_IOV
+static struct pcistub_device *find_vfs(const struct pci_dev *pdev)
+{
+ struct pcistub_device *psdev = NULL;
+ unsigned long flags;
+ bool found = false;
+
+ spin_lock_irqsave(&pcistub_devices_lock, flags);
+ list_for_each_entry(psdev, &pcistub_devices, dev_list) {
+ if (!psdev->pdev && psdev->dev != pdev
+ && pci_physfn(psdev->dev) == pdev) {
+ found = true;
+ break;
+ }
+ }
+ spin_unlock_irqrestore(&pcistub_devices_lock, flags);
+ if (found)
+ return psdev;
+ return NULL;
+}
+
+static int pci_stub_notifier(struct notifier_block *nb,
+ unsigned long action, void *data)
+{
+ struct device *dev = data;
+ const struct pci_dev *pdev = to_pci_dev(dev);
+
+ if (action != BUS_NOTIFY_UNBIND_DRIVER)
+ return NOTIFY_DONE;
+
+ if (!pdev->is_physfn)
+ return NOTIFY_DONE;
+
+ for (;;) {
+ struct pcistub_device *psdev = find_vfs(pdev);
+ if (!psdev)
+ break;
+ device_release_driver(&psdev->dev->dev);
+ }
+ return NOTIFY_DONE;
+}
+
+static struct notifier_block pci_stub_nb = {
+ .notifier_call = pci_stub_notifier,
+};
+#endif
+
static int __init xen_pcibk_init(void)
{
int err;
@@ -1539,12 +1586,19 @@ static int __init xen_pcibk_init(void)
err = xen_pcibk_xenbus_register();
if (err)
pcistub_exit();
+#ifdef CONFIG_PCI_IOV
+ else
+ bus_register_notifier(&pci_bus_type, &pci_stub_nb);
+#endif
return err;
}
static void __exit xen_pcibk_cleanup(void)
{
+#ifdef CONFIG_PCI_IOV
+ bus_unregister_notifier(&pci_bus_type, &pci_stub_nb);
+#endif
xen_pcibk_xenbus_unregister();
pcistub_exit();
}
--
1.9.3
>
> Jan
>
>>> Konrad Rzeszutek Wilk <[email protected]> 12/02/14 4:05 PM >>>
>On Tue, Dec 02, 2014 at 10:10:11AM +0000, Jan Beulich wrote:
>> >>> On 21.11.14 at 23:17, <[email protected]> wrote:
>> > Konrad Rzeszutek Wilk (7):
>> > xen/pciback: Don't deadlock when unbinding.
>> > driver core: Provide an wrapper around the mutex to do lockdep warnings
>> > xen/pciback: Include the domain id if removing the device whilst still in use
>> > xen/pciback: Print out the domain owning the device.
>> > xen/pciback: Remove tons of dereferences
>> > PCI: Expose pci_load_saved_state for public consumption.
>> > xen/pciback: Restore configuration space when detaching from a guest.
>>
>> So my "xen-pciback: drop SR-IOV VFs when PF driver unloads" isn't
>> among them, nor is there any alternative. What's the status of that
>> patch (or the problem that prompted its creation)?
>
>Oh, I've it in my queue. Um, here is what I did to it - I hadn't
>yet tested it - hence the reason it is not on that list.
Yeah, if you like it better that way, it looks okay to me now.
Jan
On 11/21/2014 05:17 PM, Konrad Rzeszutek Wilk wrote:
> The commit "xen/pciback: Don't deadlock when unbinding." was using
> the version of pci_reset_function which would lock the device lock.
> That is no good as we can dead-lock. As such we swapped to using
> the lock-less version and requiring that the callers
> of 'pcistub_put_pci_dev' take the device lock. And as such
> this bug got exposed.
>
> Using the lock-less version is OK, except that we tried to
> use 'pci_restore_state' after the lock-less version of
> __pci_reset_function_locked - which won't work as 'state_saved'
> is set to false. Said 'state_saved' is a toggle boolean that
> is to be used by the sequence of a) pci_save_state/pci_restore_state
> or b) pci_load_and_free_saved_state/pci_restore_state. We don't
> want to use a) as the guest might have messed up the PCI
> configuration space and we want it to revert to the state
> when the PCI device was binded to us. Therefore we pick
> b) to restore the configuration space.
Doesn't this all mean that patch 1/7 broke pcistub_put_pci_dev()?
-boris
>
> We restore from our 'golden' version of PCI configuration space, when an:
> - Device is unbinded from pciback
> - Device is detached from a guest.
>
> Reported-by: Sander Eikelenboom <[email protected]>
> Signed-off-by: Konrad Rzeszutek Wilk <[email protected]>
> ---
> drivers/xen/xen-pciback/pci_stub.c | 20 ++++++++++++++++----
> 1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
> index 843a2ba..eb8b58e 100644
> --- a/drivers/xen/xen-pciback/pci_stub.c
> +++ b/drivers/xen/xen-pciback/pci_stub.c
> @@ -105,7 +105,7 @@ static void pcistub_device_release(struct kref *kref)
> */
> __pci_reset_function_locked(dev);
> if (pci_load_and_free_saved_state(dev, &dev_data->pci_saved_state))
> - dev_dbg(&dev->dev, "Could not reload PCI state\n");
> + dev_info(&dev->dev, "Could not reload PCI state\n");
> else
> pci_restore_state(dev);
>
> @@ -257,6 +257,8 @@ void pcistub_put_pci_dev(struct pci_dev *dev)
> {
> struct pcistub_device *psdev, *found_psdev = NULL;
> unsigned long flags;
> + struct xen_pcibk_dev_data *dev_data;
> + int ret;
>
> spin_lock_irqsave(&pcistub_devices_lock, flags);
>
> @@ -279,9 +281,19 @@ void pcistub_put_pci_dev(struct pci_dev *dev)
> * (so it's ready for the next domain)
> */
> device_lock_assert(&dev->dev);
> - __pci_reset_function_locked(dev);
> - pci_restore_state(dev);
> -
> + dev_data = pci_get_drvdata(dev);
> + ret = pci_load_saved_state(dev, dev_data->pci_saved_state);
> + if (ret < 0)
> + dev_warn(&dev->dev, "Could not reload PCI state\n");
> + else {
> + __pci_reset_function_locked(dev);
> + /*
> + * The usual sequence is pci_save_state & pci_restore_state
> + * but the guest might have messed the configuration space up.
> + * Use the initial version (when device was bound to us).
> + */
> + pci_restore_state(dev);
> + }
> /* This disables the device. */
> xen_pcibk_reset_device(dev);
>
On Tue, Dec 02, 2014 at 06:11:50PM -0500, Boris Ostrovsky wrote:
> On 11/21/2014 05:17 PM, Konrad Rzeszutek Wilk wrote:
> >The commit "xen/pciback: Don't deadlock when unbinding." was using
> >the version of pci_reset_function which would lock the device lock.
> >That is no good as we can dead-lock. As such we swapped to using
> >the lock-less version and requiring that the callers
> >of 'pcistub_put_pci_dev' take the device lock. And as such
> >this bug got exposed.
> >
> >Using the lock-less version is OK, except that we tried to
> >use 'pci_restore_state' after the lock-less version of
> >__pci_reset_function_locked - which won't work as 'state_saved'
> >is set to false. Said 'state_saved' is a toggle boolean that
> >is to be used by the sequence of a) pci_save_state/pci_restore_state
> >or b) pci_load_and_free_saved_state/pci_restore_state. We don't
> >want to use a) as the guest might have messed up the PCI
> >configuration space and we want it to revert to the state
> >when the PCI device was binded to us. Therefore we pick
> >b) to restore the configuration space.
>
>
> Doesn't this all mean that patch 1/7 broke pcistub_put_pci_dev()?
It fixed it (there was a deadlock there).
But the fix to the dead-lock exposed this bug.
One could say that 1/7 broke it because it never worked in the
first place, but now that it works (thanks to #1) - it did not
work right?
Squashing the patches together is a bit too much I fear.
>
> -boris
>
>
> >
> >We restore from our 'golden' version of PCI configuration space, when an:
> > - Device is unbinded from pciback
> > - Device is detached from a guest.
> >
> >Reported-by: Sander Eikelenboom <[email protected]>
> >Signed-off-by: Konrad Rzeszutek Wilk <[email protected]>
> >---
> > drivers/xen/xen-pciback/pci_stub.c | 20 ++++++++++++++++----
> > 1 file changed, 16 insertions(+), 4 deletions(-)
> >
> >diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
> >index 843a2ba..eb8b58e 100644
> >--- a/drivers/xen/xen-pciback/pci_stub.c
> >+++ b/drivers/xen/xen-pciback/pci_stub.c
> >@@ -105,7 +105,7 @@ static void pcistub_device_release(struct kref *kref)
> > */
> > __pci_reset_function_locked(dev);
> > if (pci_load_and_free_saved_state(dev, &dev_data->pci_saved_state))
> >- dev_dbg(&dev->dev, "Could not reload PCI state\n");
> >+ dev_info(&dev->dev, "Could not reload PCI state\n");
> > else
> > pci_restore_state(dev);
> >@@ -257,6 +257,8 @@ void pcistub_put_pci_dev(struct pci_dev *dev)
> > {
> > struct pcistub_device *psdev, *found_psdev = NULL;
> > unsigned long flags;
> >+ struct xen_pcibk_dev_data *dev_data;
> >+ int ret;
> > spin_lock_irqsave(&pcistub_devices_lock, flags);
> >@@ -279,9 +281,19 @@ void pcistub_put_pci_dev(struct pci_dev *dev)
> > * (so it's ready for the next domain)
> > */
> > device_lock_assert(&dev->dev);
> >- __pci_reset_function_locked(dev);
> >- pci_restore_state(dev);
> >-
> >+ dev_data = pci_get_drvdata(dev);
> >+ ret = pci_load_saved_state(dev, dev_data->pci_saved_state);
> >+ if (ret < 0)
> >+ dev_warn(&dev->dev, "Could not reload PCI state\n");
> >+ else {
> >+ __pci_reset_function_locked(dev);
> >+ /*
> >+ * The usual sequence is pci_save_state & pci_restore_state
> >+ * but the guest might have messed the configuration space up.
> >+ * Use the initial version (when device was bound to us).
> >+ */
> >+ pci_restore_state(dev);
> >+ }
> > /* This disables the device. */
> > xen_pcibk_reset_device(dev);
>