On Wed, Nov 19, 2014 at 12:12:09PM -0300, Simon Martin wrote:
> Hello Jan and Konrad,
>
> Tuesday, November 18, 2014, 1:49:13 PM, you wrote:
>
> >>
> >> I've just checked this with lspci. I see that the IO is being enabled.
>
> > Memory you mean.
>
> Yes. Sorry.
>
> >> Any other idea on why I might be reading back 0xff for all PCI
> >> memory area reads? The lspci output follows.
>
> > Since this isn't behind a bridge - no, not really. Did you try this with
> > any other device for comparison purposes?
>
> This is getting more interesting. It seems that something is
> overwriting the pci-back configuration data.
>
> Starting from a fresh reboot I checked the Dom0 pci configuration and
> got this:
>
> root@smartin-xen:~# lspci -s 00:19.0 -x
> 00:19.0 Ethernet controller: Intel Corporation Device 1559 (rev 04)
> 00: 86 80 59 15 00 00 10 00 04 00 00 02 00 00 00 00
> 10: 00 00 d0 f7 00 c0 d3 f7 81 f0 00 00 00 00 00 00
> 20: 00 00 00 00 00 00 00 00 00 00 00 00 86 80 54 20
> 30: 00 00 00 00 c8 00 00 00 00 00 00 00 05 01 00 00
>
> I then start/stop my DomU and checked the Dom0 pci configuration again
> and got this:
>
> root@smartin-xen:~# lspci -s 00:19.0 -x
> 00:19.0 Ethernet controller: Intel Corporation Device 1559 (rev 04)
> 00: 86 80 59 15 00 00 10 00 04 00 00 02 00 00 00 00
> 10: 00 00 00 00 00 00 00 00 01 00 00 00 00 00 00 00
> 20: 00 00 00 00 00 00 00 00 00 00 00 00 86 80 54 20
> 30: 00 00 00 00 c8 00 00 00 00 00 00 00 05 01 00 00
>
> Inside my DomU I added code to print the PCI configuration registers
> and what I get after restarting the DomU is:
>
> (d18) 14:57:04.042 src/e1000e.c@00150: 00: 86 80 59 15 00 00 10 00 04 00 00
> 02 00 00 00 00
> (d18) 14:57:04.042 src/e1000e.c@00150: 10: 00 00 d0 f7 00 c0 d3 f7 81 f0 00
> 00 00 00 00 00
> (d18) 14:57:04.042 src/e1000e.c@00150: 20: 00 00 00 00 00 00 00 00 00 00 00
> 00 86 80 54 20
> (d18) 14:57:04.043 src/e1000e.c@00150: 30: 00 00 00 00 c8 00 00 00 00 00 00
> 00 14 01 00 00
> (d18) 14:57:04.043 src/e1000e.c@00324: Enable PCI Memory Access
> (d18) 14:57:05.043 src/e1000e.c@00150: 00: 86 80 59 15 03 00 10 00 04 00 00
> 02 00 00 00 00
> (d18) 14:57:05.044 src/e1000e.c@00150: 10: 00 00 d0 f7 00 c0 d3 f7 81 f0 00
> 00 00 00 00 00
> (d18) 14:57:05.044 src/e1000e.c@00150: 20: 00 00 00 00 00 00 00 00 00 00 00
> 00 86 80 54 20
> (d18) 14:57:05.045 src/e1000e.c@00150: 30: 00 00 00 00 c8 00 00 00 00 00 00
> 00 14 01 00 00
>
> As you can see the pci configuration read from the pci-back driver by
> my DomU is different to the data in the Dom0 pci configuration!
>
> Just before leaving my DomU I disable the pci memory access and this
> is what I see
>
> (d18) 15:01:02.051 src/e1000e.c@00150: 00: 86 80 59 15 03 00 10 00 04 00 00
> 02 00 00 00 00
> (d18) 15:01:02.051 src/e1000e.c@00150: 10: 00 00 d0 f7 00 c0 d3 f7 81 f0 00
> 00 00 00 00 00
> (d18) 15:01:02.051 src/e1000e.c@00150: 20: 00 00 00 00 00 00 00 00 00 00 00
> 00 86 80 54 20
> (d18) 15:01:02.052 src/e1000e.c@00150: 30: 00 00 00 00 c8 00 00 00 00 00 00
> 00 14 01 00 00
> (d18) 15:01:02.052 src/e1000e.c@00541: Disable PCI Memory Access
> (d18) 15:01:02.052 src/e1000e.c@00150: 00: 86 80 59 15 00 00 10 00 04 00 00
> 02 00 00 00 00
> (d18) 15:01:02.052 src/e1000e.c@00150: 10: 00 00 d0 f7 00 c0 d3 f7 81 f0 00
> 00 00 00 00 00
> (d18) 15:01:02.052 src/e1000e.c@00150: 20: 00 00 00 00 00 00 00 00 00 00 00
> 00 86 80 54 20
> (d18) 15:01:02.053 src/e1000e.c@00150: 30: 00 00 00 00 c8 00 00 00 00 00 00
> 00 14 01 00 00
>
> As you can see the data is consistent with just writing 0000 to the
> pci control register.
>
> This is the output from the debug version of the xen-pciback module.
>
> [ 5429.351231] pciback 0000:00:19.0: enabling device (0000 -> 0003)
> [ 5429.351367] xen: registering gsi 20 triggering 0 polarity 1
> [ 5429.351373] Already setup the GSI :20
> [ 5429.351387] pciback 0000:00:19.0: xen-pciback[0000:00:19.0]: #20 on
> disable-> enable
> [ 5429.351436] pciback 0000:00:19.0: xen-pciback[0000:00:19.0]: #20 on
> enabled
> [ 5434.360078] pciback 0000:00:19.0: xen-pciback[0000:00:19.0]: #20 off
> enable-> disable
> [ 5434.360116] pciback 0000:00:19.0: xen-pciback[0000:00:19.0]: #0 off
> disabled
> [ 5434.361491] xen-pciback pci-20-0: fe state changed 5
> [ 5434.362473] xen-pciback pci-20-0: fe state changed 6
> [ 5434.363540] xen-pciback pci-20-0: fe state changed 0
> [ 5434.363544] xen-pciback pci-20-0: frontend is gone! unregister device
> [ 5434.467359] pciback 0000:00:19.0: resetting virtual configuration space
> [ 5434.467376] pciback 0000:00:19.0: free-ing dynamically allocated virtual
> configuration space fields
>
> Does this make any sense to you?
There was a bug in Xen pcibackend that I thought I upstreamed which could
be releated. It was not restoring the right registers to the PCI-device.
They are attached.
>
> --
> Best regards,
> Simon mailto:furryfutt...@gmail.com
>
>From b5935d70083123aae48e115c7ed027a0ca79657f Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.w...@oracle.com>
Date: Mon, 14 Jul 2014 12:18:52 -0400
Subject: [PATCH 01/10] xen/pciback: Don't deadlock when unbinding.
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 <boris.ostrov...@oracle.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.w...@oracle.com>
Signed-off-by: David Vrabel <david.vra...@citrix.com>
---
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.8.4.2
>From 01e3c0895b0c9ee957b09f946429226c241bb5d2 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.w...@oracle.com>
Date: Wed, 6 Aug 2014 16:21:32 -0400
Subject: [PATCH 06/10] xen/pciback: Restore configuration space when detaching
from a guest.
The commit 9eea3f7695226f9af9992cebf8e98ac0ad78b277
"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.
To still retain the PCI configuration space, we save it once
more and store it on our private copy to be restored when:
- Device is unbinded from pciback
- Device is detached from a guest.
Reported-by: Sander Eikelenboom <li...@eikelenboom.it>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.w...@oracle.com>
---
drivers/xen/xen-pciback/pci_stub.c | 25 +++++++++++++++++++++----
1 file changed, 21 insertions(+), 4 deletions(-)
diff --git a/drivers/xen/xen-pciback/pci_stub.c
b/drivers/xen/xen-pciback/pci_stub.c
index 843a2ba..1126c5a 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,7 @@ 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;
spin_lock_irqsave(&pcistub_devices_lock, flags);
@@ -279,9 +280,25 @@ 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);
+ if (pci_load_and_free_saved_state(dev, &dev_data->pci_saved_state))
+ dev_info(&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 binded to us).
+ */
+ pci_restore_state(dev);
+ /*
+ * The next steps are to reload the configuration for the
+ * next time we bind & unbind to a guest - or unload from
+ * pciback.
+ */
+ pci_save_state(dev);
+ dev_data->pci_saved_state = pci_store_saved_state(dev);
+ }
/* This disables the device. */
xen_pcibk_reset_device(dev);
--
1.8.4.2
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
