Xen devel - Jan 2012 - [PATCH] Support Function Level Reset (FLR) in the xen-pciback module (v1) and some fixes.

The attached patches allow the pciback module to perform a reset whenever
a PCI device is:
 - attached to the pciback module, as so:
  echo "0000:01.07.0" > /sys/bus/pci/devices/pciback/bind
 - detached from the pciback module, as so:
  echo "0000:01.07.0" > /sys/bus/pci/devices/pciback/unbind
 - and when the guest is done with (internally when the guest is not using
   the PCI device anymore).

I ran in one issue which is that I could not do pci_reset_function call when
"bind"
or "unbind" were done as the device_lock was held (and
pci_reset_function tried to
acquire the mutex). The solution was to introduce a new
"pci_reset_function":

[PATCH 1/5] pci: Introduce __pci_reset_function_locked to be used
and then piggyback on that in
[PATCH 2/5] xen/pciback: Support pci_reset_function, aka FLR or D3

Also there are two fixes included in this - one where the PCI_DEV_FLAG_ASSIGNED
was in the wrong location and the "device has been assigned to other
domain"
warning that always appeared. More details are in the patches themselves.


 drivers/pci/pci.c                  |   25 ++++++++++++++++++++
 drivers/xen/xen-pciback/pci_stub.c |   45 +++++++++++++++++++++++++++++++++--
 drivers/xen/xen-pciback/pciback.h  |    1 +
 drivers/xen/xen-pciback/xenbus.c   |    8 +++---
 include/linux/pci.h                |    1 +
 5 files changed, 73 insertions(+), 7 deletions(-)

The use case of this is when a driver wants to call FLR when a device
is attached to it using the SysFS "bind" or "unbind"
functionality.

The call chain when a user does "bind" looks as so:

 echo "0000:01.07.0" > /sys/bus/pci/drivers/XXXX/bind

and ends up calling:
  driver_bind:
    device_lock(dev);  <=== TAKES LOCK
    XXXX_probe:
         .. pci_enable_device()
         ...__pci_reset_function(), which calls
                 pci_dev_reset(dev, 0):
                        if (!0) {
                                device_lock(dev) <==== DEADLOCK

The __pci_reset_function_locked function allows the the drivers
''probe'' function to call the "pci_reset_function"
while still holding
the driver mutex lock.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/pci/pci.c   |   25 +++++++++++++++++++++++++
 include/linux/pci.h |    1 +
 2 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 6d4a531..f9a2a0d 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3020,6 +3020,31 @@ int __pci_reset_function(struct pci_dev *dev)
 EXPORT_SYMBOL_GPL(__pci_reset_function);
 
 /**
+ * __pci_reset_function_locked - reset a PCI device function while holding
+ * the @dev mutex lock.
+ * @dev: PCI device to reset
+ *
+ * Some devices allow an individual function to be reset without affecting
+ * other functions in the same device.  The PCI device must be responsive
+ * to PCI config space in order to use this function.
+ *
+ * The device function is presumed to be unused and the caller is holding
+ * the device mutex lock when this function is called.
+ * Resetting the device will make the contents of PCI configuration space
+ * random, so any caller of this must be prepared to reinitialise the
+ * device including MSI, bus mastering, BARs, decoding IO and memory spaces,
+ * etc.
+ *
+ * Returns 0 if the device function was successfully reset or negative if the
+ * device doesn''t support resetting a single function.
+ */
+int __pci_reset_function_locked(struct pci_dev *dev)
+{
+	return pci_dev_reset(dev, 1);
+}
+EXPORT_SYMBOL_GPL(__pci_reset_function_locked);
+
+/**
  * pci_probe_reset_function - check whether the device can be safely reset
  * @dev: PCI device to reset
  *
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 7cda65b..af5ee43 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -814,6 +814,7 @@ int pcie_set_readrq(struct pci_dev *dev, int rq);
 int pcie_get_mps(struct pci_dev *dev);
 int pcie_set_mps(struct pci_dev *dev, int mps);
 int __pci_reset_function(struct pci_dev *dev);
+int __pci_reset_function_locked(struct pci_dev *dev);
 int pci_reset_function(struct pci_dev *dev);
 void pci_update_resource(struct pci_dev *dev, int resno);
 int __must_check pci_assign_resource(struct pci_dev *dev, int i);
-- 
1.7.7.4

We use the __pci_reset_function_locked to perform the action.
Also on attaching ("bind") and detaching ("unbind") we save
and
restore the configuration states. When the device is disconnected
from a guest we use the "pci_reset_function" to also reset the
device before being passed to another guest.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/xen/xen-pciback/pci_stub.c |   41 +++++++++++++++++++++++++++++++++--
 drivers/xen/xen-pciback/pciback.h  |    1 +
 2 files changed, 39 insertions(+), 3 deletions(-)

diff --git a/drivers/xen/xen-pciback/pci_stub.c
b/drivers/xen/xen-pciback/pci_stub.c
index 8f06e1e..0ce0dc6 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -85,19 +85,34 @@ static struct pcistub_device *pcistub_device_alloc(struct
pci_dev *dev)
 static void pcistub_device_release(struct kref *kref)
 {
 	struct pcistub_device *psdev;
+	struct xen_pcibk_dev_data *dev_data;
 
 	psdev = container_of(kref, struct pcistub_device, kref);
+	dev_data = pci_get_drvdata(psdev->dev);
 
 	dev_dbg(&psdev->dev->dev, "pcistub_device_release\n");
 
 	xen_unregister_device_domain_owner(psdev->dev);
 
-	/* Clean-up the device */
+	/* Call the reset function which does not take lock as this
+	 * is called from "unbind" which takes a device_lock mutex.
+	 */
+	__pci_reset_function_locked(psdev->dev);
+	if (pci_load_and_free_saved_state(psdev->dev,
+					  &dev_data->pci_saved_state)) {
+		dev_dbg(&psdev->dev->dev, "Could not reload PCI
state\n");
+	} else
+		pci_restore_state(psdev->dev);
+
+	/* Disable the device */
 	xen_pcibk_reset_device(psdev->dev);
+
+	kfree(dev_data);
+	pci_set_drvdata(psdev->dev, NULL);
+
+	/* Clean-up the device */
 	xen_pcibk_config_free_dyn_fields(psdev->dev);
 	xen_pcibk_config_free_dev(psdev->dev);
-	kfree(pci_get_drvdata(psdev->dev));
-	pci_set_drvdata(psdev->dev, NULL);
 
 	pci_dev_put(psdev->dev);
 
@@ -230,7 +245,17 @@ 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);
+	pci_restore_state(psdev->dev);
+
+	/* This disables the device. */
 	xen_pcibk_reset_device(found_psdev->dev);
+
+	/* And cleanup up our emulated fields. */
 	xen_pcibk_config_free_dyn_fields(found_psdev->dev);
 	xen_pcibk_config_reset_dev(found_psdev->dev);
 
@@ -325,6 +350,16 @@ static int __devinit pcistub_init_device(struct pci_dev
*dev)
 	if (err)
 		goto config_release;
 
+	dev_dbg(&dev->dev, "reseting (FLR, D3, etc) the device\n");
+	__pci_reset_function_locked(dev);
+
+	/* We need the device active to save the state. */
+	dev_dbg(&dev->dev, "save state of device\n");
+	pci_save_state(dev);
+	dev_data->pci_saved_state = pci_store_saved_state(dev);
+	if (!dev_data->pci_saved_state)
+		dev_err(&dev->dev, "Could not store PCI conf saved
state!\n");
+
 	/* Now disable the device (this also ensures some private device
 	 * data is setup before we export)
 	 */
diff --git a/drivers/xen/xen-pciback/pciback.h
b/drivers/xen/xen-pciback/pciback.h
index e9b4011..a7def01 100644
--- a/drivers/xen/xen-pciback/pciback.h
+++ b/drivers/xen/xen-pciback/pciback.h
@@ -41,6 +41,7 @@ struct xen_pcibk_device {
 
 struct xen_pcibk_dev_data {
 	struct list_head config_fields;
+	struct pci_saved_state *pci_saved_state;
 	unsigned int permissive:1;
 	unsigned int warned_on_write:1;
 	unsigned int enable_intx:1;
-- 
1.7.7.4

operation instead of doing it per guest creation/disconnection. Without
this we could have potentially unloaded the vf driver from the
xen pciback control even if the driver was binded to the xen-pciback.
This will hold on to it until the user "unbind"s the PCI device using
SysFS.

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

diff --git a/drivers/xen/xen-pciback/pci_stub.c
b/drivers/xen/xen-pciback/pci_stub.c
index 0ce0dc6..d66328e 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -114,6 +114,7 @@ static void pcistub_device_release(struct kref *kref)
 	xen_pcibk_config_free_dyn_fields(psdev->dev);
 	xen_pcibk_config_free_dev(psdev->dev);
 
+	psdev->dev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED;
 	pci_dev_put(psdev->dev);
 
 	kfree(psdev);
@@ -366,6 +367,7 @@ static int __devinit pcistub_init_device(struct pci_dev
*dev)
 	dev_dbg(&dev->dev, "reset device\n");
 	xen_pcibk_reset_device(dev);
 
+	dev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
 	return 0;
 
 config_release:
diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
index 0755259..474d52e 100644
--- a/drivers/xen/xen-pciback/xenbus.c
+++ b/drivers/xen/xen-pciback/xenbus.c
@@ -241,7 +241,6 @@ static int xen_pcibk_export_device(struct xen_pcibk_device
*pdev,
 		goto out;
 
 	dev_dbg(&dev->dev, "registering for %d\n",
pdev->xdev->otherend_id);
-	dev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
 	if (xen_register_device_domain_owner(dev,
 					     pdev->xdev->otherend_id) != 0) {
 		dev_err(&dev->dev, "device has been assigned to another " \
@@ -281,7 +280,6 @@ static int xen_pcibk_remove_device(struct xen_pcibk_device
*pdev,
 	}
 
 	dev_dbg(&dev->dev, "unregistering for %d\n",
pdev->xdev->otherend_id);
-	dev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED;
 	xen_unregister_device_domain_owner(dev);
 
 	xen_pcibk_release_pci_dev(pdev, dev);
-- 
1.7.7.4

When a PCI device is transferred to another domain and it is still
in usage (from the internal perspective), mention which other
domain is using it to aid in debugging.

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

diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
index 474d52e..fa130bd 100644
--- a/drivers/xen/xen-pciback/xenbus.c
+++ b/drivers/xen/xen-pciback/xenbus.c
@@ -243,8 +243,10 @@ static int xen_pcibk_export_device(struct xen_pcibk_device
*pdev,
 	dev_dbg(&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, "device has been assigned to another " \
-			"domain! Over-writting the ownership, but beware.\n");
+		int other_domain = xen_find_device_domain_owner(dev);
+		dev_err(&dev->dev, "device has been assigned to %d " \
+			"domain! Over-writting the ownership, but beware.\n",
+			other_domain);
 		xen_unregister_device_domain_owner(dev);
 		xen_register_device_domain_owner(dev, pdev->xdev->otherend_id);
 	}
-- 
1.7.7.4

The full warning is:
"pciback 0000:05:00.0: device has been assigned to 2 domain! Over-writting
the ownership, but beware."

which is correct - the previous domain that was using the device
forgot to unregister the ownership. This patch fixes this by
calling the unregister ownership function when the PCI device is
relinquished from the guest domain.

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

diff --git a/drivers/xen/xen-pciback/pci_stub.c
b/drivers/xen/xen-pciback/pci_stub.c
index d66328e..6f63b9d 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -260,6 +260,8 @@ void pcistub_put_pci_dev(struct pci_dev *dev)
 	xen_pcibk_config_free_dyn_fields(found_psdev->dev);
 	xen_pcibk_config_reset_dev(found_psdev->dev);
 
+	xen_unregister_device_domain_owner(found_psdev->dev);
+
 	spin_lock_irqsave(&found_psdev->lock, flags);
 	found_psdev->pdev = NULL;
 	spin_unlock_irqrestore(&found_psdev->lock, flags);
-- 
1.7.7.4

On Thu, 2012-01-05 at 00:46 +0000, Konrad Rzeszutek Wilk
wrote:
> We use the __pci_reset_function_locked to perform the action.
> Also on attaching ("bind") and detaching ("unbind") we
save and
> restore the configuration states. When the device is disconnected
> from a guest we use the "pci_reset_function" to also reset the
> device before being passed to another guest.
Currently I thought the toolstack was supposed to do the reset (by
writing to the reset node in sysfs) as it was (de)assigning devices
to/from guests. That''s not to say that there isn''t an argument
for
preferring to do it kernels side but it would be interesting to make
that argument explicitly.

I guess the toolstack doesn''t currently save/restore the configuration
state, could it though? I guess the state is all available in sysfs. On
the other hand the kernel provides us with a very handy function which
Just Does It...

Ian.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  drivers/xen/xen-pciback/pci_stub.c |   41
+++++++++++++++++++++++++++++++++--
>  drivers/xen/xen-pciback/pciback.h  |    1 +
>  2 files changed, 39 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/xen/xen-pciback/pci_stub.c
b/drivers/xen/xen-pciback/pci_stub.c
> index 8f06e1e..0ce0dc6 100644
> --- a/drivers/xen/xen-pciback/pci_stub.c
> +++ b/drivers/xen/xen-pciback/pci_stub.c
> @@ -85,19 +85,34 @@ static struct pcistub_device
*pcistub_device_alloc(struct pci_dev *dev)
>  static void pcistub_device_release(struct kref *kref)
>  {
>  	struct pcistub_device *psdev;
> +	struct xen_pcibk_dev_data *dev_data;
>  
>  	psdev = container_of(kref, struct pcistub_device, kref);
> +	dev_data = pci_get_drvdata(psdev->dev);
>  
>  	dev_dbg(&psdev->dev->dev,
"pcistub_device_release\n");
>  
>  	xen_unregister_device_domain_owner(psdev->dev);
>  
> -	/* Clean-up the device */
> +	/* Call the reset function which does not take lock as this
> +	 * is called from "unbind" which takes a device_lock mutex.
> +	 */
> +	__pci_reset_function_locked(psdev->dev);
> +	if (pci_load_and_free_saved_state(psdev->dev,
> +					  &dev_data->pci_saved_state)) {
> +		dev_dbg(&psdev->dev->dev, "Could not reload PCI
state\n");
> +	} else
> +		pci_restore_state(psdev->dev);
> +
> +	/* Disable the device */
>  	xen_pcibk_reset_device(psdev->dev);
> +
> +	kfree(dev_data);
> +	pci_set_drvdata(psdev->dev, NULL);
> +
> +	/* Clean-up the device */
>  	xen_pcibk_config_free_dyn_fields(psdev->dev);
>  	xen_pcibk_config_free_dev(psdev->dev);
> -	kfree(pci_get_drvdata(psdev->dev));
> -	pci_set_drvdata(psdev->dev, NULL);
>  
>  	pci_dev_put(psdev->dev);
>  
> @@ -230,7 +245,17 @@ 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);
> +	pci_restore_state(psdev->dev);
> +
> +	/* This disables the device. */
>  	xen_pcibk_reset_device(found_psdev->dev);
> +
> +	/* And cleanup up our emulated fields. */
>  	xen_pcibk_config_free_dyn_fields(found_psdev->dev);
>  	xen_pcibk_config_reset_dev(found_psdev->dev);
>  
> @@ -325,6 +350,16 @@ static int __devinit pcistub_init_device(struct
pci_dev *dev)
>  	if (err)
>  		goto config_release;
>  
> +	dev_dbg(&dev->dev, "reseting (FLR, D3, etc) the
device\n");
> +	__pci_reset_function_locked(dev);
> +
> +	/* We need the device active to save the state. */
> +	dev_dbg(&dev->dev, "save state of device\n");
> +	pci_save_state(dev);
> +	dev_data->pci_saved_state = pci_store_saved_state(dev);
> +	if (!dev_data->pci_saved_state)
> +		dev_err(&dev->dev, "Could not store PCI conf saved
state!\n");
> +
>  	/* Now disable the device (this also ensures some private device
>  	 * data is setup before we export)
>  	 */
> diff --git a/drivers/xen/xen-pciback/pciback.h
b/drivers/xen/xen-pciback/pciback.h
> index e9b4011..a7def01 100644
> --- a/drivers/xen/xen-pciback/pciback.h
> +++ b/drivers/xen/xen-pciback/pciback.h
> @@ -41,6 +41,7 @@ struct xen_pcibk_device {
>  
>  struct xen_pcibk_dev_data {
>  	struct list_head config_fields;
> +	struct pci_saved_state *pci_saved_state;
>  	unsigned int permissive:1;
>  	unsigned int warned_on_write:1;
>  	unsigned int enable_intx:1;

On 01/05/2012 05:24 AM, Ian Campbell wrote:
> On Thu, 2012-01-05 at 00:46 +0000, Konrad Rzeszutek Wilk wrote:
>> We use the __pci_reset_function_locked to perform the action.
>> Also on attaching ("bind") and detaching ("unbind")
we save and
>> restore the configuration states. When the device is disconnected
>> from a guest we use the "pci_reset_function" to also reset
the
>> device before being passed to another guest.
>
> Currently I thought the toolstack was supposed to do the reset (by
> writing to the reset node in sysfs) as it was (de)assigning devices
> to/from guests. That''s not to say that there isn''t an
argument for
> preferring to do it kernels side but it would be interesting to make
> that argument explicitly.
>
> I guess the toolstack doesn''t currently save/restore the
configuration
> state, could it though? I guess the state is all available in sysfs. On
pci_reset_function() saves the config state before doing a fcn-level reset.

the libvirt toolstack handles the unbind/reset/bind functionality
and is used by kvm for it''s device assignment.
> the other hand the kernel provides us with a very handy function which
> Just Does It...
>
> Ian.
>
>>
>> Signed-off-by: Konrad Rzeszutek Wilk<konrad.wilk@oracle.com>
>> ---
>>   drivers/xen/xen-pciback/pci_stub.c |   41
+++++++++++++++++++++++++++++++++--
>>   drivers/xen/xen-pciback/pciback.h  |    1 +
>>   2 files changed, 39 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/xen/xen-pciback/pci_stub.c
b/drivers/xen/xen-pciback/pci_stub.c
>> index 8f06e1e..0ce0dc6 100644
>> --- a/drivers/xen/xen-pciback/pci_stub.c
>> +++ b/drivers/xen/xen-pciback/pci_stub.c
>> @@ -85,19 +85,34 @@ static struct pcistub_device
*pcistub_device_alloc(struct pci_dev *dev)
>>   static void pcistub_device_release(struct kref *kref)
>>   {
>>   	struct pcistub_device *psdev;
>> +	struct xen_pcibk_dev_data *dev_data;
>>
>>   	psdev = container_of(kref, struct pcistub_device, kref);
>> +	dev_data = pci_get_drvdata(psdev->dev);
>>
>>   	dev_dbg(&psdev->dev->dev,
"pcistub_device_release\n");
>>
>>   	xen_unregister_device_domain_owner(psdev->dev);
>>
>> -	/* Clean-up the device */
>> +	/* Call the reset function which does not take lock as this
>> +	 * is called from "unbind" which takes a device_lock mutex.
>> +	 */
>> +	__pci_reset_function_locked(psdev->dev);
>> +	if (pci_load_and_free_saved_state(psdev->dev,
>> +					&dev_data->pci_saved_state)) {
>> +		dev_dbg(&psdev->dev->dev, "Could not reload PCI
state\n");
>> +	} else
>> +		pci_restore_state(psdev->dev);
>> +
>> +	/* Disable the device */
>>   	xen_pcibk_reset_device(psdev->dev);
>> +
>> +	kfree(dev_data);
>> +	pci_set_drvdata(psdev->dev, NULL);
>> +
>> +	/* Clean-up the device */
>>   	xen_pcibk_config_free_dyn_fields(psdev->dev);
>>   	xen_pcibk_config_free_dev(psdev->dev);
>> -	kfree(pci_get_drvdata(psdev->dev));
>> -	pci_set_drvdata(psdev->dev, NULL);
>>
>>   	pci_dev_put(psdev->dev);
>>
>> @@ -230,7 +245,17 @@ 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);
>> +	pci_restore_state(psdev->dev);
>> +
>> +	/* This disables the device. */
>>   	xen_pcibk_reset_device(found_psdev->dev);
>> +
>> +	/* And cleanup up our emulated fields. */
>>   	xen_pcibk_config_free_dyn_fields(found_psdev->dev);
>>   	xen_pcibk_config_reset_dev(found_psdev->dev);
>>
>> @@ -325,6 +350,16 @@ static int __devinit pcistub_init_device(struct
pci_dev *dev)
>>   	if (err)
>>   		goto config_release;
>>
>> +	dev_dbg(&dev->dev, "reseting (FLR, D3, etc) the
device\n");
>> +	__pci_reset_function_locked(dev);
>> +
>> +	/* We need the device active to save the state. */
>> +	dev_dbg(&dev->dev, "save state of device\n");
>> +	pci_save_state(dev);
>> +	dev_data->pci_saved_state = pci_store_saved_state(dev);
>> +	if (!dev_data->pci_saved_state)
>> +		dev_err(&dev->dev, "Could not store PCI conf saved
state!\n");
>> +
>>   	/* Now disable the device (this also ensures some private device
>>   	 * data is setup before we export)
>>   	 */
>> diff --git a/drivers/xen/xen-pciback/pciback.h
b/drivers/xen/xen-pciback/pciback.h
>> index e9b4011..a7def01 100644
>> --- a/drivers/xen/xen-pciback/pciback.h
>> +++ b/drivers/xen/xen-pciback/pciback.h
>> @@ -41,6 +41,7 @@ struct xen_pcibk_device {
>>
>>   struct xen_pcibk_dev_data {
>>   	struct list_head config_fields;
>> +	struct pci_saved_state *pci_saved_state;
>>   	unsigned int permissive:1;
>>   	unsigned int warned_on_write:1;
>>   	unsigned int enable_intx:1;
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe
linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Jan 05, 2012 at 01:58:50PM -0500, Don Dutile
wrote:
> On 01/05/2012 05:24 AM, Ian Campbell wrote:
> >On Thu, 2012-01-05 at 00:46 +0000, Konrad Rzeszutek Wilk wrote:
> >>We use the __pci_reset_function_locked to perform the action.
> >>Also on attaching ("bind") and detaching
("unbind") we save and
> >>restore the configuration states. When the device is disconnected
> >>from a guest we use the "pci_reset_function" to also
reset the
> >>device before being passed to another guest.
> >
> >Currently I thought the toolstack was supposed to do the reset (by
> >writing to the reset node in sysfs) as it was (de)assigning devices
> >to/from guests. That''s not to say that there isn''t an
argument for
> >preferring to do it kernels side but it would be interesting to make
> >that argument explicitly.
> >
> >I guess the toolstack doesn''t currently save/restore the
configuration
> >state, could it though? I guess the state is all available in sysfs. On
> pci_reset_function() saves the config state before doing a fcn-level reset.
> 
> the libvirt toolstack handles the unbind/reset/bind functionality
> and is used by kvm for it''s device assignment.
The KVM assigned PCI module does the call as well via the ioctls when
assigning/de-assigning the PCI device. Look in
''kvm_free_assigned_device''
and in ''kvm_vm_ioctl_assign_device''.

Is that what you mean by the unbind/reset/bind functionality - the ioctl
call?

>>> On 05.01.12 at 01:46, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> When a PCI device is transferred to another domain and it is still
> in usage (from the internal perspective), mention which other
> domain is using it to aid in debugging.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  drivers/xen/xen-pciback/xenbus.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/xen/xen-pciback/xenbus.c
b/drivers/xen/xen-pciback/xenbus.c
> index 474d52e..fa130bd 100644
> --- a/drivers/xen/xen-pciback/xenbus.c
> +++ b/drivers/xen/xen-pciback/xenbus.c
> @@ -243,8 +243,10 @@ static int xen_pcibk_export_device(struct 
> xen_pcibk_device *pdev,
>  	dev_dbg(&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, "device has been assigned to another
" \
> -			"domain! Over-writting the ownership, but beware.\n");
> +		int other_domain = xen_find_device_domain_owner(dev);
> +		dev_err(&dev->dev, "device has been assigned to %d " \
> +			"domain! Over-writting the ownership, but beware.\n",
> +			other_domain);
As you''re touching this anyway, how about fixing the other minor
issues with it too? E.g.

		dev_err(&dev->dev, "Device appears to be assigned to dom%d!"
			" Overwriting the ownership, but beware.\n",
			xen_find_device_domain_owner(dev));

(a native English speaker may want to comment the "but beware"
part - it reads odd for me).

Jan
>  		xen_unregister_device_domain_owner(dev);
>  		xen_register_device_domain_owner(dev, pdev->xdev->otherend_id);
>  	}
> -- 
> 1.7.7.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com 
> http://lists.xensource.com/xen-devel

On 06/01/12 08:38, Jan Beulich wrote:
>>>> On 05.01.12 at 01:46, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
>> When a PCI device is transferred to another domain and it is still
>> in usage (from the internal perspective), mention which other
>> domain is using it to aid in debugging.
>>
>> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> ---
>>  drivers/xen/xen-pciback/xenbus.c |    6 ++++--
>>  1 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/xen/xen-pciback/xenbus.c
b/drivers/xen/xen-pciback/xenbus.c
>> index 474d52e..fa130bd 100644
>> --- a/drivers/xen/xen-pciback/xenbus.c
>> +++ b/drivers/xen/xen-pciback/xenbus.c
>> @@ -243,8 +243,10 @@ static int xen_pcibk_export_device(struct 
>> xen_pcibk_device *pdev,
>>  	dev_dbg(&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, "device has been assigned to another
" \
>> -			"domain! Over-writting the ownership, but beware.\n");
>> +		int other_domain = xen_find_device_domain_owner(dev);
>> +		dev_err(&dev->dev, "device has been assigned to %d
" \
>> +			"domain! Over-writting the ownership, but beware.\n",
>> +			other_domain);
> As you''re touching this anyway, how about fixing the other minor
> issues with it too? E.g.
>
> 		dev_err(&dev->dev, "Device appears to be assigned to
dom%d!"
> 			" Overwriting the ownership, but beware.\n",
> 			xen_find_device_domain_owner(dev));
>
> (a native English speaker may want to comment the "but beware"
> part - it reads odd for me).
>
> Jan
Agreed - the "but beware" is superfluous to the meaning of the error
message.

~Andrew
>>  		xen_unregister_device_domain_owner(dev);
>>  		xen_register_device_domain_owner(dev,
pdev->xdev->otherend_id);
>>  	}
>> -- 
>> 1.7.7.4
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com 
>> http://lists.xensource.com/xen-devel 
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
-- 
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com

On Fri, Jan 06, 2012 at 08:38:09AM +0000, Jan Beulich
wrote:
> >>> On 05.01.12 at 01:46, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> > When a PCI device is transferred to another domain and it is still
> > in usage (from the internal perspective), mention which other
> > domain is using it to aid in debugging.
> > 
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > ---
> >  drivers/xen/xen-pciback/xenbus.c |    6 ++++--
> >  1 files changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/xen/xen-pciback/xenbus.c
b/drivers/xen/xen-pciback/xenbus.c
> > index 474d52e..fa130bd 100644
> > --- a/drivers/xen/xen-pciback/xenbus.c
> > +++ b/drivers/xen/xen-pciback/xenbus.c
> > @@ -243,8 +243,10 @@ static int xen_pcibk_export_device(struct 
> > xen_pcibk_device *pdev,
> >  	dev_dbg(&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, "device has been assigned to another
" \
> > -			"domain! Over-writting the ownership, but beware.\n");
> > +		int other_domain = xen_find_device_domain_owner(dev);
> > +		dev_err(&dev->dev, "device has been assigned to %d
" \
> > +			"domain! Over-writting the ownership, but beware.\n",
> > +			other_domain);
> 
> As you''re touching this anyway, how about fixing the other minor
> issues with it too? E.g.
> 
> 		dev_err(&dev->dev, "Device appears to be assigned to
dom%d!"
> 			" Overwriting the ownership, but beware.\n",
> 			xen_find_device_domain_owner(dev));
> 
> (a native English speaker may want to comment the "but beware"
> part - it reads odd for me).
Hm, lets ask the English speakers. Ian?

On 06/01/2012 15:03, "Konrad Rzeszutek Wilk"
<konrad.wilk@oracle.com> wrote:
>> As you''re touching this anyway, how about fixing the other
minor
>> issues with it too? E.g.
>> 
>> dev_err(&dev->dev, "Device appears to be assigned to
dom%d!"
>> " Overwriting the ownership, but beware.\n",
>> xen_find_device_domain_owner(dev));
>> 
>> (a native English speaker may want to comment the "but
beware"
>> part - it reads odd for me).
> 
> Hm, lets ask the English speakers. Ian?
The suggested reworking above is an improvement. The "but beware" bit
is
fine.

 -- keir

On Fri, 2012-01-06 at 15:03 +0000, Konrad Rzeszutek Wilk
wrote:
> On Fri, Jan 06, 2012 at 08:38:09AM +0000, Jan Beulich wrote:
> > >>> On 05.01.12 at 01:46, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> > > When a PCI device is transferred to another domain and it is
still
> > > in usage (from the internal perspective), mention which other
> > > domain is using it to aid in debugging.
> > > 
> > > Signed-off-by: Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com>
> > > ---
> > >  drivers/xen/xen-pciback/xenbus.c |    6 ++++--
> > >  1 files changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/xen/xen-pciback/xenbus.c
b/drivers/xen/xen-pciback/xenbus.c
> > > index 474d52e..fa130bd 100644
> > > --- a/drivers/xen/xen-pciback/xenbus.c
> > > +++ b/drivers/xen/xen-pciback/xenbus.c
> > > @@ -243,8 +243,10 @@ static int xen_pcibk_export_device(struct 
> > > xen_pcibk_device *pdev,
> > >  	dev_dbg(&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, "device has been assigned to
another " \
> > > -			"domain! Over-writting the ownership, but
beware.\n");
> > > +		int other_domain = xen_find_device_domain_owner(dev);
> > > +		dev_err(&dev->dev, "device has been assigned to %d
" \
> > > +			"domain! Over-writting the ownership, but
beware.\n",
> > > +			other_domain);
> > 
> > As you''re touching this anyway, how about fixing the other
minor
> > issues with it too? E.g.
> > 
> > 		dev_err(&dev->dev, "Device appears to be assigned to
dom%d!"
> > 			" Overwriting the ownership, but beware.\n",
> > 			xen_find_device_domain_owner(dev));
> > 
> > (a native English speaker may want to comment the "but
beware"
> > part - it reads odd for me).
> 
> Hm, lets ask the English speakers. Ian?
I think you underestimate a native speakers ability to mangle a
language. Especially this one ;-)

Anyway, I think it''s unnecessary, so you may as well drop it and just
have:

 		dev_err(&dev->dev, "Device appears to be assigned to dom%d!"
 			" Overwriting the ownership.\n",
 			xen_find_device_domain_owner(dev));

I suppose you might want "Overriding ownership to dom%d".

Ian.

On Fri, Jan 06, 2012 at 03:51:25PM +0000, Ian Campbell
wrote:
> On Fri, 2012-01-06 at 15:03 +0000, Konrad Rzeszutek Wilk wrote:
> > On Fri, Jan 06, 2012 at 08:38:09AM +0000, Jan Beulich wrote:
> > > >>> On 05.01.12 at 01:46, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> > > > When a PCI device is transferred to another domain and it is
still
> > > > in usage (from the internal perspective), mention which
other
> > > > domain is using it to aid in debugging.
> > > > 
> > > > Signed-off-by: Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com>
> > > > ---
> > > >  drivers/xen/xen-pciback/xenbus.c |    6 ++++--
> > > >  1 files changed, 4 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/xen/xen-pciback/xenbus.c
b/drivers/xen/xen-pciback/xenbus.c
> > > > index 474d52e..fa130bd 100644
> > > > --- a/drivers/xen/xen-pciback/xenbus.c
> > > > +++ b/drivers/xen/xen-pciback/xenbus.c
> > > > @@ -243,8 +243,10 @@ static int
xen_pcibk_export_device(struct
> > > > xen_pcibk_device *pdev,
> > > >  	dev_dbg(&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, "device has been assigned
to another " \
> > > > -			"domain! Over-writting the ownership, but
beware.\n");
> > > > +		int other_domain = xen_find_device_domain_owner(dev);
> > > > +		dev_err(&dev->dev, "device has been assigned
to %d " \
> > > > +			"domain! Over-writting the ownership, but
beware.\n",
> > > > +			other_domain);
> > > 
> > > As you''re touching this anyway, how about fixing the
other minor
> > > issues with it too? E.g.
> > > 
> > > 		dev_err(&dev->dev, "Device appears to be assigned
to dom%d!"
> > > 			" Overwriting the ownership, but beware.\n",
> > > 			xen_find_device_domain_owner(dev));
> > > 
> > > (a native English speaker may want to comment the "but
beware"
> > > part - it reads odd for me).
> > 
> > Hm, lets ask the English speakers. Ian?
> 
> I think you underestimate a native speakers ability to mangle a
> language. Especially this one ;-)
> 
> Anyway, I think it''s unnecessary, so you may as well drop it and
just
> have:
> 
>  		dev_err(&dev->dev, "Device appears to be assigned to
dom%d!"
>  			" Overwriting the ownership.\n",
>  			xen_find_device_domain_owner(dev));
> 
> I suppose you might want "Overriding ownership to dom%d".
OK. To the point and potentially can fit in 80 lines :-).

Will spin out the patch next week-sih.

On Thu, Jan 05, 2012 at 10:24:11AM +0000, Ian Campbell
wrote:
> On Thu, 2012-01-05 at 00:46 +0000, Konrad Rzeszutek Wilk wrote:
> > We use the __pci_reset_function_locked to perform the action.
> > Also on attaching ("bind") and detaching
("unbind") we save and
> > restore the configuration states. When the device is disconnected
> > from a guest we use the "pci_reset_function" to also reset
the
> > device before being passed to another guest.
> 
> Currently I thought the toolstack was supposed to do the reset (by
> writing to the reset node in sysfs) as it was (de)assigning devices
> to/from guests. That''s not to say that there isn''t an
argument for
> preferring to do it kernels side but it would be interesting to make
> that argument explicitly.
I am kind of on the fence on this one. The one issue I found with
the toolstack is that it sometimes is run too late (now mind you -
I haven''t actually found any bugs, so this might be all ballloony).

The xen-pciback can be used to inhibit drivers from grabbing the devices
during bootup. This can be done by
''xen-pciback.hide=(02:00.0)(02:00.1)(..)''

Pretty handy as you don''t have to deal with:
 echo "0000:02:00.1" >
/sys/bus/pci/devices/0000:02:00.1/drivers/unbind

and then later ''rmmod e1000e'' to save some memory space.

The unfortunate side effect is that xen-pciback does this:

 pci_enable_device()
 pci_disable_device(dev)
 pci_write_config_word(dev, PCI_COMMAND, 0)

... and then when the guest finally uses the device it would (this is what
the toolstack does - I think?):

 echo "1" > /sys/bus/pci/devices/0000:02:00.1/reset

which calls
 pci_dev_reset() - do FLR or D3, or whatnot
 pci_save_state()

 pci_write_config_word(dev, PCI_COMMAND, PCI_COMMAND_INTX_DISABLE);

 pci_dev_reset(dev) [with a lock held so that "reset" on SysFS cannot
be done]
 pci_restore_state()

which means that the PCI configuration state before the
"pci_save_state"
is already influenced by what the xen-pciback had done during startup:
the pci_enable/pci_disable/pci_write_config_work(dev, PCI_COMMAND, 0).

When the ''pci_restore_state()'' is called it would restore it
to
what xen-pciback.hide=(xx) did. Not what the device state is before xen-pciback
gets its hand on it.

This is part of this patchset - were we make sure to save it before we do our
deed.
(with those pci_save_config/pci_restore_config). 
And to be fair - I hadn''t seen any issues with this, so it might not be
neccessary.

The other way of making this work is to remove the 
pci_write_config_work(dev, PCI_COMMAND, 0) but I hadn''t seen any bugs
with
that ..
> 
> I guess the toolstack doesn''t currently save/restore the
configuration
> state, could it though? I guess the state is all available in sysfs. On
> the other hand the kernel provides us with a very handy function which
> Just Does It...
Yeah, there is one more use case - ''xm''. The
''xm'' does not do ''reset'' on the
SysFS so having it done in the kernel is kind of nice. But then
''xm'' is on the
deprecated list so xen-unstable does not care about it. But I do since
Fedora Core 16 is using it ..

On 01/05/2012 04:34 PM, Konrad Rzeszutek Wilk wrote:
> On Thu, Jan 05, 2012 at 01:58:50PM -0500, Don Dutile wrote:
>> On 01/05/2012 05:24 AM, Ian Campbell wrote:
>>> On Thu, 2012-01-05 at 00:46 +0000, Konrad Rzeszutek Wilk wrote:
>>>> We use the __pci_reset_function_locked to perform the action.
>>>> Also on attaching ("bind") and detaching
("unbind") we save and
>>>> restore the configuration states. When the device is
disconnected
>>> >from a guest we use the "pci_reset_function" to also
reset the
>>>> device before being passed to another guest.
>>>
>>> Currently I thought the toolstack was supposed to do the reset (by
>>> writing to the reset node in sysfs) as it was (de)assigning devices
>>> to/from guests. That''s not to say that there
isn''t an argument for
>>> preferring to do it kernels side but it would be interesting to
make
>>> that argument explicitly.
>>>
>>> I guess the toolstack doesn''t currently save/restore the
configuration
>>> state, could it though? I guess the state is all available in
sysfs. On
>> pci_reset_function() saves the config state before doing a fcn-level
reset.
>>
>> the libvirt toolstack handles the unbind/reset/bind functionality
>> and is used by kvm for it''s device assignment.
>
> The KVM assigned PCI module does the call as well via the ioctls when
> assigning/de-assigning the PCI device. Look in
''kvm_free_assigned_device''
> and in ''kvm_vm_ioctl_assign_device''.
>
> Is that what you mean by the unbind/reset/bind functionality - the ioctl
> call?
>
It''s done in both places (libvirt & kvm-ioctl).

> > I suppose you might want "Overriding ownership to dom%d".
> 
> OK. To the point and potentially can fit in 80 lines :-).
how about this?
> 
From a3d4a80cdfd4274016522572148a89260b3f3de6 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Wed, 4 Jan 2012 14:16:45 -0500
Subject: [PATCH] xen/pciback: Expand the warning message to include domain
 id.

When a PCI device is transferred to another domain and it is still
in usage (from the internal perspective), mention which other
domain is using it to aid in debugging.

[v2: Truncate the verbose message per Jan Beulich suggestion]
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/xen/xen-pciback/xenbus.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
index 474d52e..2405a24 100644
--- a/drivers/xen/xen-pciback/xenbus.c
+++ b/drivers/xen/xen-pciback/xenbus.c
@@ -243,8 +243,8 @@ static int xen_pcibk_export_device(struct xen_pcibk_device
*pdev,
 	dev_dbg(&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, "device has been assigned to another " \
-			"domain! Over-writting the ownership, but beware.\n");
+		dev_err(&dev->dev, "Overriding ownership to dom%d.\n",
+			xen_find_device_domain_owner(dev));
 		xen_unregister_device_domain_owner(dev);
 		xen_register_device_domain_owner(dev, pdev->xdev->otherend_id);
 	}
-- 
1.7.7.4

>>> On 06.01.12 at 23:19, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
>> > I suppose you might want "Overriding ownership to
dom%d".
>> 
>> OK. To the point and potentially can fit in 80 lines :-).
> 
> how about this?
>> 
> From a3d4a80cdfd4274016522572148a89260b3f3de6 Mon Sep 17 00:00:00 2001
> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Date: Wed, 4 Jan 2012 14:16:45 -0500
> Subject: [PATCH] xen/pciback: Expand the warning message to include domain
>  id.
> 
> When a PCI device is transferred to another domain and it is still
> in usage (from the internal perspective), mention which other
> domain is using it to aid in debugging.
> 
> [v2: Truncate the verbose message per Jan Beulich suggestion]
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
> ---
>  drivers/xen/xen-pciback/xenbus.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/xen/xen-pciback/xenbus.c
b/drivers/xen/xen-pciback/xenbus.c
> index 474d52e..2405a24 100644
> --- a/drivers/xen/xen-pciback/xenbus.c
> +++ b/drivers/xen/xen-pciback/xenbus.c
> @@ -243,8 +243,8 @@ static int xen_pcibk_export_device(struct 
> xen_pcibk_device *pdev,
>  	dev_dbg(&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, "device has been assigned to another
" \
> -			"domain! Over-writting the ownership, but beware.\n");
> +		dev_err(&dev->dev, "Overriding ownership to dom%d.\n",
> +			xen_find_device_domain_owner(dev));
>  		xen_unregister_device_domain_owner(dev);
>  		xen_register_device_domain_owner(dev, pdev->xdev->otherend_id);
>  	}
> -- 
> 1.7.7.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com 
> http://lists.xensource.com/xen-devel

On Fri, 2012-01-06 at 22:19 +0000, Konrad Rzeszutek Wilk
wrote:
> > > I suppose you might want "Overriding ownership to
dom%d".
> > 
> > OK. To the point and potentially can fit in 80 lines :-).
> 
> how about this?
> > 
> From a3d4a80cdfd4274016522572148a89260b3f3de6 Mon Sep 17 00:00:00 2001
> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Date: Wed, 4 Jan 2012 14:16:45 -0500
> Subject: [PATCH] xen/pciback: Expand the warning message to include domain
>  id.
> 
> When a PCI device is transferred to another domain and it is still
> in usage (from the internal perspective), mention which other
> domain is using it to aid in debugging.
> 
> [v2: Truncate the verbose message per Jan Beulich suggestion]
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  drivers/xen/xen-pciback/xenbus.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/xen/xen-pciback/xenbus.c
b/drivers/xen/xen-pciback/xenbus.c
> index 474d52e..2405a24 100644
> --- a/drivers/xen/xen-pciback/xenbus.c
> +++ b/drivers/xen/xen-pciback/xenbus.c
> @@ -243,8 +243,8 @@ static int xen_pcibk_export_device(struct
xen_pcibk_device *pdev,
>  	dev_dbg(&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, "device has been assigned to another
" \
> -			"domain! Over-writting the ownership, but beware.\n");
> +		dev_err(&dev->dev, "Overriding ownership to dom%d.\n",
> +			xen_find_device_domain_owner(dev));
That sounds like you are going to be assigning the ownership to that
dom, but xen_find_device_domain_owner so aren''t you actually steeling
ownership from that domain?
>  		xen_unregister_device_domain_owner(dev);
>  		xen_register_device_domain_owner(dev, pdev->xdev->otherend_id);
>  	}

On Mon, Jan 09, 2012 at 09:50:57AM +0000, Ian Campbell
wrote:
> On Fri, 2012-01-06 at 22:19 +0000, Konrad Rzeszutek Wilk wrote:
> > > > I suppose you might want "Overriding ownership to
dom%d".
> > > 
> > > OK. To the point and potentially can fit in 80 lines :-).
> > 
> > how about this?
> > > 
> > From a3d4a80cdfd4274016522572148a89260b3f3de6 Mon Sep 17 00:00:00 2001
> > From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Date: Wed, 4 Jan 2012 14:16:45 -0500
> > Subject: [PATCH] xen/pciback: Expand the warning message to include
domain
> >  id.
> > 
> > When a PCI device is transferred to another domain and it is still
> > in usage (from the internal perspective), mention which other
> > domain is using it to aid in debugging.
> > 
> > [v2: Truncate the verbose message per Jan Beulich suggestion]
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > ---
> >  drivers/xen/xen-pciback/xenbus.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/xen/xen-pciback/xenbus.c
b/drivers/xen/xen-pciback/xenbus.c
> > index 474d52e..2405a24 100644
> > --- a/drivers/xen/xen-pciback/xenbus.c
> > +++ b/drivers/xen/xen-pciback/xenbus.c
> > @@ -243,8 +243,8 @@ static int xen_pcibk_export_device(struct
xen_pcibk_device *pdev,
> >  	dev_dbg(&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, "device has been assigned to another
" \
> > -			"domain! Over-writting the ownership, but beware.\n");
> > +		dev_err(&dev->dev, "Overriding ownership to
dom%d.\n",
> > +			xen_find_device_domain_owner(dev));
> 
> That sounds like you are going to be assigning the ownership to that
> dom, but xen_find_device_domain_owner so aren''t you actually
steeling
> ownership from that domain?
Duh! I will do s/to/from/

On Mon, 2012-01-09 at 15:11 +0000, Konrad Rzeszutek Wilk
wrote:
> On Mon, Jan 09, 2012 at 09:50:57AM +0000, Ian Campbell wrote:
> > On Fri, 2012-01-06 at 22:19 +0000, Konrad Rzeszutek Wilk wrote:
> > > > > I suppose you might want "Overriding ownership to
dom%d".
> > > > 
> > > > OK. To the point and potentially can fit in 80 lines :-).
> > > 
> > > how about this?
> > > > 
> > > From a3d4a80cdfd4274016522572148a89260b3f3de6 Mon Sep 17 00:00:00
2001
> > > From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > > Date: Wed, 4 Jan 2012 14:16:45 -0500
> > > Subject: [PATCH] xen/pciback: Expand the warning message to
include domain
> > >  id.
> > > 
> > > When a PCI device is transferred to another domain and it is
still
> > > in usage (from the internal perspective), mention which other
> > > domain is using it to aid in debugging.
> > > 
> > > [v2: Truncate the verbose message per Jan Beulich suggestion]
> > > Signed-off-by: Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com>
> > > ---
> > >  drivers/xen/xen-pciback/xenbus.c |    4 ++--
> > >  1 files changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/xen/xen-pciback/xenbus.c
b/drivers/xen/xen-pciback/xenbus.c
> > > index 474d52e..2405a24 100644
> > > --- a/drivers/xen/xen-pciback/xenbus.c
> > > +++ b/drivers/xen/xen-pciback/xenbus.c
> > > @@ -243,8 +243,8 @@ static int xen_pcibk_export_device(struct
xen_pcibk_device *pdev,
> > >  	dev_dbg(&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, "device has been assigned to
another " \
> > > -			"domain! Over-writting the ownership, but
beware.\n");
> > > +		dev_err(&dev->dev, "Overriding ownership to
dom%d.\n",
> > > +			xen_find_device_domain_owner(dev));
> > 
> > That sounds like you are going to be assigning the ownership to that
> > dom, but xen_find_device_domain_owner so aren''t you actually
steeling
> > ownership from that domain?
> 
> Duh! I will do s/to/from/
Maybe s/Overriding/Stealing/ too? "Overriding ownership from xxx"
sounds
a bit odd to me.

Ian.

> > > > -		dev_err(&dev->dev, "device has been assigned
to another " \
> > > > -			"domain! Over-writting the ownership, but
beware.\n");
> > > > +		dev_err(&dev->dev, "Overriding ownership to
dom%d.\n",
> > > > +			xen_find_device_domain_owner(dev));
> > > 
> > > That sounds like you are going to be assigning the ownership to
that
> > > dom, but xen_find_device_domain_owner so aren''t you
actually steeling
> > > ownership from that domain?
> > 
> > Duh! I will do s/to/from/
> 
> Maybe s/Overriding/Stealing/ too? "Overriding ownership from xxx"
sounds
> a bit odd to me.
Bingo! "Stealing ownership from dom%d." it is!